refactor: replace use-subscription#6246
refactor: replace use-subscription#6246SukkaW wants to merge 2 commits intoDimensionDev:developfrom SukkaW:replace-use-subscription
Conversation
Jack-Works
left a comment
There was a problem hiding this comment.
Thanks for your interest! I have some review suggestions
| /** | ||
| * (Synchronously) returns the current value of our subscription. | ||
| */ | ||
| getCurrentValue: () => T |
There was a problem hiding this comment.
I think you should add a getServerValue?(): T to Subscription type, and export the following hooks from the shared (React hooks should not inside the shared-base) package:
function useSubscription<T>(sub: Subscription<T>): T {
return useSyncExternalStoreWithSelector(
sub.subscribe,
sub.getCurrentValue,
sub.getServerValue || sub.getCurrentValue,
(s) => s,
)
}It will make the migration easier.
There was a problem hiding this comment.
The approach is only good for migration, not for future refactoring.
Consider this: What happened if different logic for getServerValue would be applied across different useSyncExternalStoreWithSelector usage in the future? What happened if multiple subscriptions will be merged into one store and then fully utilize the selector feature in the future?
IMHO, we should keep useSyncExternalStoreWithSelector as it is.
There was a problem hiding this comment.
We haven't made a plan to use a full-featured store like zustand or redux, thus I think easy migration is more important.
What happened if different logic for getServerValue would be applied across different useSyncExternalStoreWithSelector usage in the future?
Can you explain this part?
Description
Closes # (NO_ISSUE)
Type of change
Checklist
console.logsIf this PR depends on external APIs:
chrome-extension://[id]moz-extension://[id]Back in 2019, React released the first version of
use-subscription(facebook/react#15022). At the time, we only have very limited information about concurrent rendering.In 2020, React provides a first-party official API
useMutableSource(reactjs/rfcs#147, facebook/react#18000):React 18 introduces
useMutableSource's replacementuseSyncExternalStore(see details here: reactwg/react-18#86), and React changesuse-subscriptionimplementation to useuseSyncExternalStoredirectly: facebook/react#24289And according to
use-subscription:The PR does exactly that:
use-subscriptionuse-sync-external-storeto the dependencies.use-sync-external-storepackage enables compatibility with React 16 and React 17.useSubscriptionusage withuseSyncExternalStoreWithSelectoruseSyncExternalStoreshim,useSyncExternalStoreWithSelectorprovides the built-in memo support to prevent infinite loop rendering.