React Context / update Zustand #746
Conversation
|
@xinaesthete We will accept the PR. I will review soon. |
|
Another thing you might want to check as you review is whether the 'equality function' stuff looks right & isn't applied in the wrong way anywhere. That part of the PR was because of Zustand warnings but I suppose could be a place where there's potential for me to've done something sub-optimal, or that might need to change yet again with subsequent Zustand update (pmndrs/zustand#1937). Anyway, if anything does need to change now or in future, it will be in a very focused part of the code I believe. |
There was a problem hiding this comment.
The changes are fairly minimal, you're right. I will look again tomorrow and do a bit more testing as well, but generally looks good. Could you just verbally confirm that nothing changes except https://github.com/hms-dbmi/viv/pull/746/files#diff-2a7404f5ffcf40cd00ee647b6e067050c477b33c83ee32e245e5484e1096b51dR158-R193 (and the exports + setting state code)? I just want to be sure I'm not missing anything since the diff is non-trivial.
sites/avivator/src/state.js
Outdated
| export const AvivatorContext = createContext(null); | ||
|
|
||
| export const AvivatorProvider = ({ children }) => { | ||
| const storesRef = useRef(null); | ||
| if (!storesRef.current) { | ||
| storesRef.current = { | ||
| channels: createChannelsStore(), | ||
| imageSettings: createImageSettingsStore(), | ||
| viewer: createViewerStore() | ||
| }; | ||
| } | ||
| return createElement( | ||
| AvivatorContext.Provider, | ||
| { value: storesRef.current }, | ||
| children | ||
| ); | ||
| }; | ||
|
|
||
| function useStoreApi(storeName) { | ||
| const store = useContext(AvivatorContext); | ||
| if (!store) throw 'useStore must be used within a AvivatorProvider'; | ||
| return store[storeName]; | ||
| } | ||
| export const useChannelsStoreApi = () => useStoreApi('channels'); | ||
| export const useImageSettingsStoreApi = () => useStoreApi('imageSettings'); | ||
| export const useViewerStoreApi = () => useStoreApi('viewer'); | ||
| function useAviStore(storeName, selector, eqFn) { | ||
| const store = useStoreApi(storeName); | ||
| return useStoreWithEqualityFn(store, selector, eqFn); | ||
| } | ||
| export const useChannelsStore = (selector, eqFn) => | ||
| useAviStore('channels', selector, eqFn); | ||
| export const useImageSettingsStore = (selector, eqFn) => | ||
| useAviStore('imageSettings', selector, eqFn); | ||
| export const useViewerStore = (selector, eqFn) => | ||
| useAviStore('viewer', selector, eqFn); |
There was a problem hiding this comment.
Can you explain the "architecture," maybe just using docstrings?
There was a problem hiding this comment.
Can you explain the "architecture," maybe just using docstrings?
Will do.
There was a problem hiding this comment.
@ilan-gold I've added some explanation, lmk if it is clear etc.
It's just occurred to me that there is probably another change needed to assign unique IDs to views - should be reasonably straightforward, although would also be quite easy to miss something - with a risk of that leaving potential obscure bugs lurking under the surface...
I think any such bugs would only apply in instances where multiple contexts exist, but you'd be very justified in seeing it as an extra potential red-flag: please don't feel you're committed to accepting these changes. I'm going to do a bit more work on the MDV side - hopefully get that into a better state than the one I'd left it in recently 🙄, probably write/copy a bit more code that I hope to delete, and get more of a sense of how useful it'd be to have more of avivator available to import...
There was a problem hiding this comment.
Locally, I've added an id to the context, with corresponding useAvivatorId() hook, and am traversing the code, reviewing what changes would be necessary... I don't have a systematic way right now of precisely identifying every place that would need to be changed - but it is fair to say that the surface-area of required changes will likely be (significantly) larger than I'd previously thought, so could well tip the balance away from this being something that should reasonably be merged... it does risk polluting parts of the code that are currently simple, even if I believe it shouldn't introduce new bugs (other than to anyone -mostly me- using unsupported features)...
I don't think it's a monumental task to get it to into a state where I'd feel confident about it - and I may carry on with that - but in no way want to push things that not everyone's happy with. As I commented elsewhere, it may actually be a better strategy for me to just treat the Avivator code as a demo from which I can borrow in a less formal way (with credit and within the license, of course).
There was a problem hiding this comment.
FWIW in case interested, this code-sandbox is a minimal version of the same pattern with TypeScript types.
Background
This refactors to use React Context, as a step towards allowing more of Avivator to be used as a library, as per #745
Change List
<AvivatorProvider>is used in index.jsxexport const useXxxStore = create(...)becomesconst createXxxStore = () => createStore(...), with correspondingusedeclarations further down. I think this change looks bigger in diff than it might otherwise because of prettier formatting.useXxxStoreApihave been added, which access the context and return a reference to the given store. This is used by new versions of theuseXxxStorehooks (which also wrapuseStoreWithEqualityFnas per deprecation warnings from Zustand), as well as anywhere thatuseXxxStore.setState(...)was previously used, as it's not possible to determine which context should be used outside React rendering (event callbacks etc).To the best of my knowledge this doesn't impact functionality or performance - pretty sure the current pattern doesn't incur any extra renders or unnecessarily expensive Zustand calls, but I could be mistaken.
I've run tests/lint etc as well as manually testing most features in the Avivator UI.
Checklist