Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React Context / update Zustand #746

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

xinaesthete
Copy link

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

  • Updated Zustand; may need review if there are issues with associated upgrade of React to 18. Would perhaps have been better to do this separately to reduce noise in this PR (various warnings from Zustand have also been addressed).
  • Adapt stores to use context API
    • An <AvivatorProvider> is used in index.jsx
    • export const useXxxStore = create(...) becomes const createXxxStore = () => createStore(...), with corresponding use declarations further down. I think this change looks bigger in diff than it might otherwise because of prettier formatting.
    • new hooks for useXxxStoreApi have been added, which access the context and return a reference to the given store. This is used by new versions of the useXxxStore hooks (which also wrap useStoreWithEqualityFn as per deprecation warnings from Zustand), as well as anywhere that useXxxStore.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

  • Update JSdoc types if there is any API change. n/a?
  • Make sure Avivator works as expected with your change.

@ilan-gold
Copy link
Collaborator

@xinaesthete We will accept the PR. I will review soon.

@xinaesthete
Copy link
Author

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.

@ilan-gold ilan-gold self-requested a review November 30, 2023 14:13
Copy link
Collaborator

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 158 to 193
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the "architecture," maybe just using docstrings?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the "architecture," maybe just using docstrings?

Will do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW in case interested, this code-sandbox is a minimal version of the same pattern with TypeScript types.

@xinaesthete xinaesthete marked this pull request as draft July 3, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants