fix: change useStateStore to use useSyncExternalStore #2573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arnautov-anton
requested review from
oliverlaz,
myandrienko,
isekovanic and
MartinCupela
|
Size Change: +1.43 kB (+0.12%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2573 +/- ##
==========================================
+ Coverage 81.12% 81.15% +0.02%
==========================================
Files 448 448
Lines 9464 9479 +15
Branches 2312 2316 +4
==========================================
+ Hits 7678 7693 +15
Misses 1423 1423
Partials 363 363 ☔ View full report in Codecov by Sentry. |
arnautov-anton
force-pushed
the
fix/use-sync-external-store
branch
2 times, most recently
from
897e7fd to
eb4d195
Compare
myandrienko
reviewed
Dec 9, 2024
src/store/hooks/useStateStore.ts
Outdated
| @@ -1,7 +1,10 @@ | |||
| import { useEffect, useState } from 'react'; | |||
| import { useCallback, useMemo, useSyncExternalStore } from 'react'; | |||
There was a problem hiding this comment.
To support older React versions, it's better to use the use-sync-external-store/shim package. It also has a useSyncExternalStoreWithSelector helper hook which will do selector and selection memoization for you.
There was a problem hiding this comment.
Thank you, I'll use the shim but the one with selector still requires you to provide a comparator for the selected values so I'll keep it the way it is as it seems that re-writing it to fit the other API would yield similar results - or as would James Franco say:
There was a problem hiding this comment.
Do I understand it well, that not using use-sync-external-store/shim could lead to new GH issues of those, who are using React 16 @myandrienko ?
There was a problem hiding this comment.
For the ones who have React version smaller than React 18.
There was a problem hiding this comment.
That's correct, you should use use-sync-external-store/shim if you want to support react@<18. It uses the "native" implementation on react@>=18, so there's no real downside.
arnautov-anton
force-pushed
the
fix/use-sync-external-store
branch
from
442c7c7 to
1c36b8c
Compare
arnautov-anton
force-pushed
the
fix/use-sync-external-store
branch
2 times, most recently
from
1e97101 to
0977dfd
Compare
arnautov-anton
force-pushed
the
fix/use-sync-external-store
branch
from
0977dfd to
856eda3
Compare
arnautov-anton
changed the title
arnautov-anton
force-pushed
the
fix/use-sync-external-store
branch
from
856eda3 to
1b1fb35
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎯 Goal
Changing stores on the fly would keep previously calculated state for a bit before the effect would run to recalculate it - using
useSyncExternalStore(thank you, @myandrienko) should alleviate this issue. BothsubscribeandgetSnapshotfunctions required by the React hook are wrapped to allow for selector functionality,geSnapshotrequires the output to be cached so the wrapper reuses similar cache check mechanism assubscribeWithSelectordoes internally.