Initial shim of useSyncExternalStore #22211
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
acdlite
force-pushed
the
initial-use-sync-external-store-shim
branch
3 times, most recently
from
a3000c5
to
44b073b
Compare
|
Comparing: 3385b37...505c398 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
acdlite
force-pushed
the
initial-use-sync-external-store-shim
branch
8 times, most recently
from
365cee7
to
27f0d23
Compare
| // When we initially subscribe, we need to check if there was a mutation in | ||
| // between render and commit. If there was, we may need to re-render. | ||
| // Subsequent changes will be detected in the subscription handler. | ||
| const valueDuringRender = value; |
There was a problem hiding this comment.
This shim doesn't need to support strict effects because that doesn't exist in versions <18.
However, if it did, then this value would represent the value during the initial subscribe, but during a "reconnect" (e.g. Offscreen going from hidden back to visible) it needs to represent the rendered value when we disconnected.
So, it should read from the ref, like we do in the subscription handler.
Same logic applies to getSnapshot.
So I think that means we can run exactly the same code when checking for tearing on subscribe and when receiving a subscription event.
| inst.value = nextValue; | ||
| setVersion(bumpVersion); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
What errors do we expect here? Is it proper error or also errors that happen due to tearing that can be fixed? Should this intentionally silence errors or no?
There was a problem hiding this comment.
Since we also schedule an update, it will throw again during render so you can catch it with an error boundary. Same thing that we do for reducers.
| } | ||
|
|
||
| // Subscribe to the store and return a clean-up function. | ||
| return subscribe(handleStoreChange); |
There was a problem hiding this comment.
This should move to useEffect.
| // directly in render. | ||
| // | ||
| // We force a re-render by bumping a version number. | ||
| const [, setVersion] = useState(0); |
There was a problem hiding this comment.
If we stored the snapshot in the state, and didn't use it for anything else. That would still enable certain types of bailouts to happen in React itself. Would that lead to the correct behavior? Would it give us any benefits?
It at least avoids the possibility of something updating more than long times (highly unlikely). :D
There was a problem hiding this comment.
Yeah that makes sense. I was thinking we could also enable lazy bailouts by using an inline reducer. If during render we detect that the state hasn’t changed, return the previous state. Otherwise return a new one. Could flip a Boolean back and forth.
There was a problem hiding this comment.
I don’t think it can literally be the snapshot though since the subscription could change and then you have to disregard the queued updates some how.
There was a problem hiding this comment.
Or rather, it might bail out incorrectly and not force an update.
acdlite
force-pushed
the
initial-use-sync-external-store-shim
branch
2 times, most recently
from
7b44303
to
9e9df92
Compare
This sets up an initial shim implementation of useSyncExternalStore, via the use-sync-external-store package. It's designed to mimic the behavior of the built-in API, but is backwards compatible to any version of React that supports hooks. I have not yet implemented the built-in API, but once it exists, the use-sync-external-store package will always prefer that one. Library authors can depend on the shim and trust that their users get the correct implementation. See reactwg/react-18#86 for background on the API. The tests I've added here are designed to run against both the shim and built-in implementation, using our variant test flag feature. Tests that only apply to concurrent roots will live in a separate suite.
acdlite
force-pushed
the
initial-use-sync-external-store-shim
branch
from
9e9df92
to
505c398
Compare
|
One question: how does the |
|
Yeah since it can be implemented pretty easily in userspace, we're not planning to add it to the built-in API. That way for cases where a custom But since we expect it will be a frequent request, we went ahead and implemented our own. If you don't need a custom Another alternative to // Option 1: Separate subscription per field
const a = useSyncExternalStore(store.subscribe, () => store.getState().a);
const b = useSyncExternalStore(store.subscribe, () => store.getState().b);
// Option 2: One subscription for multiple fields
const {a, b} = useSyncExternalStoreExtra(
store.subscribe,
store.getState,
state => ({ a: state.a, b: state.b }),
shallowEqual,
);The "extra" naming is a placeholder, not sure what we'll end up calling it. |
If this is true for both react 17 and 18, "extra" part doesn't have to be in use-sync-external-store package, and lib authors can do the same.
I can imagine some use cases fall into this pattern. |
|
I'm starting to play around with this for React-Redux v8 right now. I'll leave a couple notes here for lack of somewhere better, and I can create a new issue if desired:
it('reuse latest selected state on selector re-run', () => {
const alwaysEqual = () => true
const Comp = () => {
// triggers render on store change
useNormalSelector((s) => s.count)
const array = useSelector(() => [1, 2, 3], alwaysEqual)
useLayoutEffect(() => {
renderedItems.push(array)
})
return <div />
}
rtl.render(
<ProviderMock store={normalStore}>
<Comp />
</ProviderMock>
)
expect(renderedItems.length).toBe(1)
act(() => {
normalStore.dispatch({ type: '' })
})
expect(renderedItems.length).toBe(2)
expect(renderedItems[0]).toBe(renderedItems[1])
})If anyone's interested, here's my current PR: and specific current commit is reduxjs/react-redux@8da5607 |
Summary: This sync includes the following changes: - **[95d762e40](facebook/react@95d762e40 )**: Remove duplicate test //<Andrew Clark>// - **[d4d1dc085](facebook/react@d4d1dc085 )**: Reorder VARIANT feature flags ([#22266](facebook/react#22266)) //<Dan Abramov>// - **[2f156eafb](facebook/react@2f156eafb )**: Adjust consoleManagedByDevToolsDuringStrictMode feature flag ([#22253](facebook/react#22253)) //<Dan Abramov>// - **[cfd819332](facebook/react@cfd819332 )**: Add useSyncExternalStore to react-debug-tools ([#22240](facebook/react#22240)) //<Andrew Clark>// - **[8e80592a3](facebook/react@8e80592a3 )**: Remove state queue from useSyncExternalStore ([#22265](facebook/react#22265)) //<Andrew Clark>// - **[06f98c168](facebook/react@06f98c168 )**: Implement useSyncExternalStore in Fiber ([#22239](facebook/react#22239)) //<Andrew Clark>// - **[77912d9a0](facebook/react@77912d9a0 )**: Wire up the native API for useSyncExternalStore ([#22237](facebook/react#22237)) //<Andrew Clark>// - **[031abd24b](facebook/react@031abd24b )**: Add warning and test for useSyncExternalStore when getSnapshot isn't cached ([#22262](facebook/react#22262)) //<salazarm>// - **[b8884de24](facebook/react@b8884de24 )**: break up import keyword to avoid being accidentally parsed as dynamic import statement in external code ([#21918](facebook/react#21918)) //<Jianhua Zheng>// - **[6d6bba5bf](facebook/react@6d6bba5bf )**: Fix typo in ReactUpdatePriority-test.js ([#21958](facebook/react#21958)) //<Ikko Ashimine>// - **[0c0d1ddae](facebook/react@0c0d1ddae )**: feat(eslint-plugin-react-hooks): support ESLint 8.x ([#22248](facebook/react#22248)) //<Michaël De Boey>// - **[1314299c7](facebook/react@1314299c7 )**: Initial shim of useSyncExternalStore ([#22211](facebook/react#22211)) //<Andrew Clark>// - **[fc40f02ad](facebook/react@fc40f02ad )**: Add consoleManagedByDevToolsDuringStrictMode feature flag in React Reconciler ([#22196](facebook/react#22196)) //<Luna Ruan>// - **[46a0f050a](facebook/react@46a0f050a )**: Set up use-sync-external-store package ([#22202](facebook/react#22202)) //<Andrew Clark>// - **[8723e772b](facebook/react@8723e772b )**: Fix a string interpolation typo in ReactHooks test ([#22174](facebook/react#22174)) //<Matt Hargett>// - **[60a30cf32](facebook/react@60a30cf32 )**: Console Logging for StrictMode Double Rendering ([#22030](facebook/react#22030)) //<Luna Ruan>// - **[76bbad3e3](facebook/react@76bbad3e3 )**: Add maxYieldMs feature flag in Scheduler ([#22165](facebook/react#22165)) //<Ricky>// - **[b0b53ae2c](facebook/react@b0b53ae2c )**: Add feature flags for scheduler experiments ([#22105](facebook/react#22105)) //<Ricky>// Changelog: [General][Changed] - React Native sync for revisions bd5bf55...95d762e jest_e2e[run_all_tests] Reviewed By: mdvacca Differential Revision: D30809906 fbshipit-source-id: 131cfdf91e15f67fa59a5d925467e538ee89fe10
This sets up an initial shim implementation of useSyncExternalStore, via the use-sync-external-store package. It's designed to mimic the behavior of the built-in API, but is backwards compatible to any version of React that supports hooks. I have not yet implemented the built-in API, but once it exists, the use-sync-external-store package will always prefer that one. Library authors can depend on the shim and trust that their users get the correct implementation. See reactwg/react-18#86 for background on the API. The tests I've added here are designed to run against both the shim and built-in implementation, using our variant test flag feature. Tests that only apply to concurrent roots will live in a separate suite.
This sets up an initial shim implementation of useSyncExternalStore, via the use-sync-external-store package. It's designed to mimic the behavior of the built-in API, but is backwards compatible to any version of React that supports hooks.
I have not yet implemented the built-in API, but once it exists, the use-sync-external-store package will always prefer that one. Library authors can depend on the shim and trust that their users get the correct implementation.
See reactwg/react-18#86 for background on the API.
The tests I've added here are designed to run against both the shim and built-in implementation, using our variant test flag feature. Tests that only apply to concurrent roots will live in a separate suite.
Things to consider before landing:
getSSRSnapshot). Probably should be a required argument.isEqual? I called ituseSyncExternalStoreExtraas a placeholder.