-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
Unmemoized AnnotoriousContext
state forces too frequent re-renders for its children
#401
Comments
Well, in fact I'd say this works as - currently - designed. When you create a new selection, this will instantly create an annotation. Any change to the text selection (while you drag) will update the currently selected annotation. That naturally means you'd get a re-render, because there's an actual state change going on that child components might want to react on. I believe the only solution is (as we discussed) to not create the annotation instantly, but only on first pointerup. I couldn't yet look into this though. Might have repercussions on the mobile compatibility, too. |
Exactly 👌🏻 And that's where the |
Issue
By default, w/o memoization, React makes all the children that read the context's state re-render upon any change. That what's happening rn for
AnnotoriousContext.Provider
:annotorious/packages/annotorious-react/src/Annotorious.tsx
Lines 97 to 106 in e112aee
When I select some text on a page and use the
useAnnotationStore
, or any other hook with underlyinguseContext(AnnotoriousContext)
in a component - it'll get re-rendered on every update.Screen.Recording.2024-05-13.at.14.31.57.mov
Even though it can be expected that the
store
, which exposes only the callbacks, will be stable during that process 🤔That's not very significant for individual components, but when you have a dozen+ notes that read from the A9S store - any new selection feels slower.
Suggested Solutions / Steps
TL;DR;
I haven't come up with a single solution that would be generic enough and not be too opinionated on the underlying tech 🤔
Here are some thoughts on that matter:
Unfortunately, it's not a viable option to follow the official optimization guide and wrap the state into the
useMemo
, because the highly volatileselection
prop will still lead to updates. It'll also trigger updates for theanno
prop through theanno.state.selection.selected
.A much better alternative would be to use state-management lib like
zustand
. It saves the state in aref
and operates upon the pub-sub model. So it'll help read only a certain state part and prevent excessive re-renders. But it's bit of an opinionated solution that might get into the versions conflict with the consuming apps 🤔The text was updated successfully, but these errors were encountered: