chore: Internal reactive store utils #145
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #145 +/- ##
==========================================
+ Coverage 99.38% 99.41% +0.02%
==========================================
Files 33 34 +1
Lines 810 850 +40
Branches 214 229 +15
==========================================
+ Hits 805 845 +40
Misses 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pan-kot
requested review from
YueyingLu and
connorlanigan
and removed request for
a team
| useEffect( | ||
| () => { | ||
| if (prevStore.current !== store) { | ||
| effect(selector(store.get()), selector(prevStore.current.get())); |
There was a problem hiding this comment.
Here the effect runs if the store changed, even if the actual state hasn't changed. Either this should not happen (my preference) or the function's documentation should be updated to inform users of this hook about this.
| // Ignoring selector and effect as they are expected to stay constant. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
This should either be documented for users of the hook, or adjusted so that they do not have to stay constant. This could likely be achieved by using the useStableCallback function. The same applies to the useSelector hook below
|
|
||
| // We create subscription synchronously during the first render cycle to ensure the store updates that | ||
| // happen after the first render but before the first effect are not lost. | ||
| const unsubscribeRef = useRef(store.subscribe(selector, newState => setState(selector(newState)))); |
There was a problem hiding this comment.
This will create a new subscription on every render, which will likely be bad for performance over time
Rel: [GNQwANHlIGi6]
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.