Separate from react #56
Conversation
lib/core/hash.js
Outdated
There was a problem hiding this comment.
Wonder how performant this is? Compared to something like https://www.npmjs.com/package/object-hash ?
| * A helper to split the properties into a query descriptor `desc` (including 'params') | ||
| * and figbird-specific query configuration `config` | ||
| */ | ||
| export function splitConfig(combinedConfig) { |
There was a problem hiding this comment.
IMO it's clearer to keep the figbird/feathers properties in separate objects than try and merge the 2. Prevents a potential issue (although probably quite small), where a feathers update adds a property that conflicts with a figbird one
There was a problem hiding this comment.
but also this would be a breaking change and it's not a big deal
There was a problem hiding this comment.
agreed, I think we can break backwards compat in the future, but wanted to make it work in the current codebase! so this splitConfig is a bit of stop gap to contain this issue
| }) | ||
| } | ||
|
|
||
| #subscribeToRealtime(queryId) { |
There was a problem hiding this comment.
sometimes this returns an unsubscribe fn but if it's already subscribed it will return undefined. And it looks like where this is called it never uses the returned value so maybe missing an unsubscribe call somewhere?
If the idea is to subscribe once but allow multiple calls might need a reference counter?
There was a problem hiding this comment.
Yeah, this bit a bit unfinished - the idea right now is that we never unsubscribe from relatime (and never delete anything from the cache) - once you ran the query, it keeps consuming realtime events. How to unsub/evict - not currently defined in. This is not far off from what the app is already doing, so I don't see this to be a big issue.
|
|
||
| const shouldVacuumByDefault = q.config.fetchPolicy === 'network-only' | ||
| return ({ vacuum = shouldVacuumByDefault } = {}) => { | ||
| removeListener() |
There was a problem hiding this comment.
I think maybe here we should unsubscribe realtime too?
There was a problem hiding this comment.
But then queries can get stale, for no good reason? Why not keep integrating those events if the websocket is receiving them?
There was a problem hiding this comment.
I see. But if the unsubscribe actually only stopped the listening when all queries have torn down then I think it makes sense, as there would be no queries using them?
Maybe at the point vacuum runs below, as that's clearing out the query anyway so it will need to be fetched again next time it's used?
|
|
||
| const shouldVacuumByDefault = q.config.fetchPolicy === 'network-only' | ||
| return ({ vacuum = shouldVacuumByDefault } = {}) => { | ||
| removeListener() |
There was a problem hiding this comment.
I see. But if the unsubscribe actually only stopped the listening when all queries have torn down then I think it makes sense, as there would be no queries using them?
Maybe at the point vacuum runs below, as that's clearing out the query anyway so it will need to be fetched again next time it's used?
| const refetch = useCallback(() => q.refetch(), [q]) | ||
| const subscribe = useCallback(fn => q.subscribe(fn), [q]) | ||
| const getSnapshot = useCallback(() => q.getSnapshot(), [q]) |
There was a problem hiding this comment.
if q is stable, then q.refetch, q.subscribe, and q.getSnapshot will also be stable, no need to wrap them in useCallbacks?
| dispatch({ type: 'mutating' }) | ||
| try { | ||
| const item = await figbird.mutate({ serviceName, method, args }) | ||
| if (mountedRef.current) { |
Yet to be tested in the real app! Need to rewire where the app reads the cache from for the global atom.