feat: DH-16737 Add ObjectManager, useWidget hook
#2030
Conversation
mofojed
changed the title
useWidget hookuseWidget hook
mofojed
changed the title
useWidget hookuseWidget hook
- Just allows for subscribing to an object, which will allow refetching when necessary
- Cleaned up the useObjectFetch tests as well - Still not using the useWidget hook, will use from deephaven-plugin-ui
mofojed
changed the title
useWidget hookuseWidget hook
| // Signal that we're still loading | ||
| setCurrentUpdate({ | ||
| fetch: null, | ||
| error: null, | ||
| }); |
There was a problem hiding this comment.
Is this necessary since the state is already set to this initially? I guess it would be if objectFetchManager changed
There was a problem hiding this comment.
Yea, I guess technically I could check if the old update is still fetch: null; error: null... it would technically matter if you are just looking at the object instead of destructuring it right away, as it's a new object...
I guess there's no real reason to fallback to the objectFetcher either. I should just take that out.
| if (objectFetchManager == null) { | ||
| if (objectFetcher == null) { | ||
| setCurrentUpdate({ | ||
| fetch: null, | ||
| error: new Error('No ObjectFetchManager available in context'), | ||
| }); | ||
| } else { | ||
| setCurrentUpdate({ | ||
| fetch: () => objectFetcher(descriptor), | ||
| error: null, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Do we want to be able to fetch without the object manager?
The error in the first block says no manager available, but that's true in both blocks
There was a problem hiding this comment.
This also makes me wonder if these 2 should be part of the same context. Is there a case where someone should be getting just the object fetcher and not using the object fetch manager?
There was a problem hiding this comment.
Hmm yea they could be combined... ObjectFetcher would be if you just want to fetch an object once, not subscribe to it and listen for updates... though not sure when you actually would want to do that.
| }; | ||
|
|
||
| /** | ||
| * Retrieve a widget for the given variable descriptor. Note that if the widget is successfully fetched, ownership of the widget is passed to the consumer and will need to close the object as well. |
There was a problem hiding this comment.
Are there frequently cases where we wouldn't want to just close this when useWidget unmounts? Wondering if the norm should be we clean it up automatically and provide a config option to not clean it up automatically if you have a good reason.
There was a problem hiding this comment.
Yea, with a widget you can get updates that include new exported objects (like we do with deephaven.ui), those need to be cleaned up as well; or you may want to re-export exported objects rather than fetch them, I don't think we want to automatically close things here as it's not clear if the objects were actually fetched by the consumer or not.
|
I think the status messages are fine (as long as they're constants). I've used tanstack query some which has each state as a boolean value in the returned object. When I was using it, it seemed to be a fine pattern as well. In the case of tanstack query it's ajax/browser fetch requests with optional refetch intervals/events. https://tanstack.com/query/latest/docs/framework/react/reference/useQuery |
- Some docs changes - Allow other types other than just `dh.Widget` to be used
- Remove type as it's already inferred - Add case to check if table is closed if unmounted before fetched
ObjectManagercontext allows for implementation to handle loading the objectObjectManager, as we just have one connectionObjectManagerprovided to the context manager will need to handle fetching from queries, and will be able to handle more scenarios (such as when a query is restarting)useWidgethook to load widgets deephaven-plugins#502