USACE / groundwork

React Components for USACE Applications
https://usace.github.io/groundwork
4 stars 0 forks source link

CDA Latest Value Card Component #33

Closed jbkolze closed 5 months ago

jbkolze commented 5 months ago

Draft of a pretty highly-abstracted card component to display the latest value and timestamp for a provided CDA time series.

I have a mess of ideas on how to lay out the props in my head, so, as promised, I'm going to try to lay them out here and see what sticks.

Right now the component just takes the two required parameters for a CDA TimeSeries request (tsId and office) and leaves the rest as default. The idea was for this to be a very high-level "plug-and-play" component. But, this means it uses a default time window of now -> 24 hours ago. And, I'm just realizing now, returns the default units... So this probably works for a lot of use cases, but sometimes it won't. So, a few options:

That's a lot of stuff so I'll leave it there. Curious to hear anyone's ideas / suggestions / opinions on what route to take with this.

willbreitkreutz commented 5 months ago

I like option 1, but instead of requiring the user to make all the decisions, let them override any of the defaults, i.e.

const CdaLatestValueCard = ({ cdaParams, tsId, office, ... }) => {
  const defaultCdaParams = {...}
  const { data, isPending, isError } = useCdaTimeSeries({
    cdaParams: { ...defaultCdaParams, ...cdaParams, ...{ name: tsId, office }},
  });
willbreitkreutz commented 5 months ago

cdaParams: { ...defaultCdaParams, ...cdaParams, ...{ name: tsId, office }},

And you could either leave tsId and office as top-level props, or just move them into cdaParams, that does make it harder to throw errors if they don't give you values, but only marginally.

jbkolze commented 5 months ago

Okay, that works for me. This is really throwing me for a loop because it feels odd to duplicate the entire data hook input within the display component, but I think that's probably just the way it'll have to be until we have some changes on the backend. Ideally I'd like to be able to use <CdaLatestValueCard tsid="foo" /> and it always just works, but that'll require officeID in the config and some API changes/updates.

Any thoughts on including useCatalog somehow, whether by default or toggled? As-is it would be dependent on the user to account for the case in which the latest value is more than 24 hours old, and I feel like a component called "LatestValueCard" should guarantee that you get the latest value. Maybe that should just be clearly stated in the docs though.

willbreitkreutz commented 5 months ago

@jbkolze a call using useCatalog might be handy, it is a double-round-trip like you said, but as long as it's relatively quick it'd be ok. too bad the timeseries endpoint doesn't let you do /timeseries?order_by=time&order_dir=desc&limit=1000 or something like that

jbkolze commented 5 months ago

Just had an idea that I think might get us the best of both worlds. By default, make the default 24-hour check -- if no data, make a useCatalog request and find the latest timestamp, then make another useTS request to get the latest value. 3 calls so very slow if the data is old, but it at least works. And then also add a toggle for "expectOld" which will just run the useCatalog first by default, so take it down to 2 requests. Going to make for some interesting conditionals but I think I can make it work once I wrap my head around it.

Re: the endpoints, I'm hoping if we actually get more people on-board using this stuff then we'll get some more traction for changes / updates to the available APIs. We'll see.

jbkolze commented 5 months ago

@willbreitkreutz Alright, was a bit of a bumpy ride but I think I'm happy with where it's at now. Fetches latest value within 24 hours if available, and if not it checks the catalog and re-fetches with the specified date.

There's a bunch of logic in this component now that should be pulled out into a useCdaLatestTimeSeries hook and potentially exposed as another available groundwork hook. Task for another day, though. Also bailed on the "expectOld" for the time being -- think it would require a lot of monkeying around with hook precedence/conditionality without much gain.

Since the component is doing a lot of stuff with the cdaParams now I removed the hook pass-through props -- I think they would just add confusion. Exposed the unit parameter, which I think is the only thing a user might potentially need to adjust.

Open to any additional suggestions or fixes, but barring that I think it's ready for review/merge.

willbreitkreutz commented 5 months ago

@jbkolze sorry for the delay, i'm going to roll in the outstanding pull requests and push a new minor version today