From f6693d72168562c394eac4d44021d036e13e5ac7 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 7 Sep 2021 13:22:05 -0400 Subject: [PATCH] Implement useSyncExternalStore in Fiber (#22239) This adds an initial implementation of useSyncExternalStore to the fiber reconciler. It's mostly a copy-paste of the userspace implementation, which is not ideal but is a good enough starting place. The main change we'll want to make to this native implementation is to move the tearing checks from the layout phase to an earlier, pre-commit phase so that code that runs in the commit phase always observes a consistent tree. Follow-ups: - Implement in Fizz - Implement in old SSR renderer - Implement in react-debug-hooks --- .../src/ReactFiberHooks.new.js | 119 ++++++++++++++- .../src/ReactFiberHooks.old.js | 119 ++++++++++++++- .../useSyncExternalStoreShared-test.js | 143 +++++++++--------- 3 files changed, 305 insertions(+), 76 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 40beec7f8c742..d36c745dae985 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -51,6 +51,7 @@ import { markRootMutableRead, } from './ReactFiberLane.new'; import { + DiscreteEventPriority, ContinuousEventPriority, getCurrentUpdatePriority, setCurrentUpdatePriority, @@ -136,6 +137,7 @@ export type UpdateQueue = {| let didWarnAboutMismatchedHooksForComponent; let didWarnAboutUseOpaqueIdentifier; +let didWarnUncachedGetSnapshot; if (__DEV__) { didWarnAboutUseOpaqueIdentifier = {}; didWarnAboutMismatchedHooksForComponent = new Set(); @@ -1246,14 +1248,127 @@ function mountSyncExternalStore( subscribe: (() => void) => () => void, getSnapshot: () => T, ): T { - throw new Error('Not yet implemented'); + const hook = mountWorkInProgressHook(); + return useSyncExternalStore(hook, subscribe, getSnapshot); } function updateSyncExternalStore( subscribe: (() => void) => () => void, getSnapshot: () => T, ): T { - throw new Error('Not yet implemented'); + const hook = updateWorkInProgressHook(); + return useSyncExternalStore(hook, subscribe, getSnapshot); +} + +function useSyncExternalStore( + hook: Hook, + subscribe: (() => void) => () => void, + getSnapshot: () => T, +): T { + // TODO: This is a copy-paste of the userspace shim. We can improve the + // built-in implementation using lower-level APIs. We also intend to move + // the tearing checks to an earlier, pre-commit phase so that the layout + // effects always observe a consistent tree. + + const dispatcher = ReactCurrentDispatcher.current; + + // Read the current snapshot from the store on every render. Again, this + // breaks the rules of React, and only works here because of specific + // implementation details, most importantly that updates are + // always synchronous. + const value = getSnapshot(); + if (__DEV__) { + if (!didWarnUncachedGetSnapshot) { + if (value !== getSnapshot()) { + console.error( + 'The result of getSnapshot should be cached to avoid an infinite loop', + ); + didWarnUncachedGetSnapshot = true; + } + } + } + + // Because updates are synchronous, we don't queue them. Instead we force a + // re-render whenever the subscribed state changes by updating an some + // arbitrary useState hook. Then, during render, we call getSnapshot to read + // the current value. + // + // Because we don't actually use the state returned by the useState hook, we + // can save a bit of memory by storing other stuff in that slot. + // + // To implement the early bailout, we need to track some things on a mutable + // object. Usually, we would put that in a useRef hook, but we can stash it in + // our useState hook instead. + // + // To force a re-render, we call forceUpdate({inst}). That works because the + // new object always fails an equality check. + const [{inst}, forceUpdate] = dispatcher.useState({ + inst: {value, getSnapshot}, + }); + + // Track the latest getSnapshot function with a ref. This needs to be updated + // in the layout phase so we can access it during the tearing check that + // happens on subscribe. + // TODO: Circumvent SSR warning + dispatcher.useLayoutEffect(() => { + inst.value = value; + inst.getSnapshot = getSnapshot; + + // Whenever getSnapshot or subscribe changes, we need to check in the + // commit phase if there was an interleaved mutation. In concurrent mode + // this can happen all the time, but even in synchronous mode, an earlier + // effect may have mutated the store. + if (checkIfSnapshotChanged(inst)) { + // Force a re-render. + const prevTransition = ReactCurrentBatchConfig.transition; + const prevPriority = getCurrentUpdatePriority(); + ReactCurrentBatchConfig.transition = 0; + setCurrentUpdatePriority(DiscreteEventPriority); + forceUpdate({inst}); + setCurrentUpdatePriority(prevPriority); + ReactCurrentBatchConfig.transition = prevTransition; + } + }, [subscribe, value, getSnapshot]); + + dispatcher.useEffect(() => { + const handleStoreChange = () => { + // TODO: Because there is no cross-renderer API for batching updates, it's + // up to the consumer of this library to wrap their subscription event + // with unstable_batchedUpdates. Should we try to detect when this isn't + // the case and print a warning in development? + + // The store changed. Check if the snapshot changed since the last time we + // read from the store. + if (checkIfSnapshotChanged(inst)) { + // Force a re-render. + const prevTransition = ReactCurrentBatchConfig.transition; + const prevPriority = getCurrentUpdatePriority(); + ReactCurrentBatchConfig.transition = 0; + setCurrentUpdatePriority(DiscreteEventPriority); + forceUpdate({inst}); + setCurrentUpdatePriority(prevPriority); + ReactCurrentBatchConfig.transition = prevTransition; + } + }; + // Check for changes right before subscribing. Subsequent changes will be + // detected in the subscription handler. + handleStoreChange(); + // Subscribe to the store and return a clean-up function. + return subscribe(handleStoreChange); + }, [subscribe]); + + return value; +} + +function checkIfSnapshotChanged(inst) { + const latestGetSnapshot = inst.getSnapshot; + const prevValue = inst.value; + try { + const nextValue = latestGetSnapshot(); + return !is(prevValue, nextValue); + } catch (error) { + return true; + } } function mountState( diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 08c61e9b162f6..c062845d6dbd5 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -51,6 +51,7 @@ import { markRootMutableRead, } from './ReactFiberLane.old'; import { + DiscreteEventPriority, ContinuousEventPriority, getCurrentUpdatePriority, setCurrentUpdatePriority, @@ -136,6 +137,7 @@ export type UpdateQueue = {| let didWarnAboutMismatchedHooksForComponent; let didWarnAboutUseOpaqueIdentifier; +let didWarnUncachedGetSnapshot; if (__DEV__) { didWarnAboutUseOpaqueIdentifier = {}; didWarnAboutMismatchedHooksForComponent = new Set(); @@ -1246,14 +1248,127 @@ function mountSyncExternalStore( subscribe: (() => void) => () => void, getSnapshot: () => T, ): T { - throw new Error('Not yet implemented'); + const hook = mountWorkInProgressHook(); + return useSyncExternalStore(hook, subscribe, getSnapshot); } function updateSyncExternalStore( subscribe: (() => void) => () => void, getSnapshot: () => T, ): T { - throw new Error('Not yet implemented'); + const hook = updateWorkInProgressHook(); + return useSyncExternalStore(hook, subscribe, getSnapshot); +} + +function useSyncExternalStore( + hook: Hook, + subscribe: (() => void) => () => void, + getSnapshot: () => T, +): T { + // TODO: This is a copy-paste of the userspace shim. We can improve the + // built-in implementation using lower-level APIs. We also intend to move + // the tearing checks to an earlier, pre-commit phase so that the layout + // effects always observe a consistent tree. + + const dispatcher = ReactCurrentDispatcher.current; + + // Read the current snapshot from the store on every render. Again, this + // breaks the rules of React, and only works here because of specific + // implementation details, most importantly that updates are + // always synchronous. + const value = getSnapshot(); + if (__DEV__) { + if (!didWarnUncachedGetSnapshot) { + if (value !== getSnapshot()) { + console.error( + 'The result of getSnapshot should be cached to avoid an infinite loop', + ); + didWarnUncachedGetSnapshot = true; + } + } + } + + // Because updates are synchronous, we don't queue them. Instead we force a + // re-render whenever the subscribed state changes by updating an some + // arbitrary useState hook. Then, during render, we call getSnapshot to read + // the current value. + // + // Because we don't actually use the state returned by the useState hook, we + // can save a bit of memory by storing other stuff in that slot. + // + // To implement the early bailout, we need to track some things on a mutable + // object. Usually, we would put that in a useRef hook, but we can stash it in + // our useState hook instead. + // + // To force a re-render, we call forceUpdate({inst}). That works because the + // new object always fails an equality check. + const [{inst}, forceUpdate] = dispatcher.useState({ + inst: {value, getSnapshot}, + }); + + // Track the latest getSnapshot function with a ref. This needs to be updated + // in the layout phase so we can access it during the tearing check that + // happens on subscribe. + // TODO: Circumvent SSR warning + dispatcher.useLayoutEffect(() => { + inst.value = value; + inst.getSnapshot = getSnapshot; + + // Whenever getSnapshot or subscribe changes, we need to check in the + // commit phase if there was an interleaved mutation. In concurrent mode + // this can happen all the time, but even in synchronous mode, an earlier + // effect may have mutated the store. + if (checkIfSnapshotChanged(inst)) { + // Force a re-render. + const prevTransition = ReactCurrentBatchConfig.transition; + const prevPriority = getCurrentUpdatePriority(); + ReactCurrentBatchConfig.transition = 0; + setCurrentUpdatePriority(DiscreteEventPriority); + forceUpdate({inst}); + setCurrentUpdatePriority(prevPriority); + ReactCurrentBatchConfig.transition = prevTransition; + } + }, [subscribe, value, getSnapshot]); + + dispatcher.useEffect(() => { + const handleStoreChange = () => { + // TODO: Because there is no cross-renderer API for batching updates, it's + // up to the consumer of this library to wrap their subscription event + // with unstable_batchedUpdates. Should we try to detect when this isn't + // the case and print a warning in development? + + // The store changed. Check if the snapshot changed since the last time we + // read from the store. + if (checkIfSnapshotChanged(inst)) { + // Force a re-render. + const prevTransition = ReactCurrentBatchConfig.transition; + const prevPriority = getCurrentUpdatePriority(); + ReactCurrentBatchConfig.transition = 0; + setCurrentUpdatePriority(DiscreteEventPriority); + forceUpdate({inst}); + setCurrentUpdatePriority(prevPriority); + ReactCurrentBatchConfig.transition = prevTransition; + } + }; + // Check for changes right before subscribing. Subsequent changes will be + // detected in the subscription handler. + handleStoreChange(); + // Subscribe to the store and return a clean-up function. + return subscribe(handleStoreChange); + }, [subscribe]); + + return value; +} + +function checkIfSnapshotChanged(inst) { + const latestGetSnapshot = inst.getSnapshot; + const prevValue = inst.value; + try { + const nextValue = latestGetSnapshot(); + return !is(prevValue, nextValue); + } catch (error) { + return true; + } } function mountState( diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js index 4bf0c4bc955af..387440c053d49 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js @@ -116,8 +116,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { }; } - // @gate !variant - test('basic usage', () => { + test('basic usage', async () => { const store = createExternalStore('Initial'); function App() { @@ -126,19 +125,18 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { } const root = createRoot(); - act(() => root.render()); + await act(() => root.render()); expect(Scheduler).toHaveYielded(['Initial']); expect(root).toMatchRenderedOutput('Initial'); - act(() => { + await act(() => { store.set('Updated'); }); expect(Scheduler).toHaveYielded(['Updated']); expect(root).toMatchRenderedOutput('Updated'); }); - // @gate !variant test('skips re-rendering if nothing changes', () => { const store = createExternalStore('Initial'); @@ -162,8 +160,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('Initial'); }); - // @gate !variant - test('switch to a different store', () => { + test('switch to a different store', async () => { const storeA = createExternalStore(0); const storeB = createExternalStore(0); @@ -176,29 +173,31 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { } const root = createRoot(); - act(() => root.render()); + await act(() => root.render()); expect(Scheduler).toHaveYielded([0]); expect(root).toMatchRenderedOutput('0'); - act(() => { + await act(() => { storeA.set(1); }); expect(Scheduler).toHaveYielded([1]); expect(root).toMatchRenderedOutput('1'); - // Switch stores + // Switch stores and update in the same batch act(() => { - // This update will be disregarded - storeA.set(2); - setStore(storeB); + ReactNoop.flushSync(() => { + // This update will be disregarded + storeA.set(2); + setStore(storeB); + }); }); // Now reading from B instead of A expect(Scheduler).toHaveYielded([0]); expect(root).toMatchRenderedOutput('0'); // Update A - act(() => { + await act(() => { storeA.set(3); }); // Nothing happened, because we're no longer subscribed to A @@ -206,15 +205,14 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('0'); // Update B - act(() => { + await act(() => { storeB.set(1); }); expect(Scheduler).toHaveYielded([1]); expect(root).toMatchRenderedOutput('1'); }); - // @gate !variant - test('selecting a specific value inside getSnapshot', () => { + test('selecting a specific value inside getSnapshot', async () => { const store = createExternalStore({a: 0, b: 0}); function A() { @@ -242,7 +240,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A0B0'); // Update b but not a - act(() => { + await act(() => { store.set({a: 0, b: 1}); }); // Only b re-renders @@ -250,7 +248,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A0B1'); // Update a but not b - act(() => { + await act(() => { store.set({a: 1, b: 1}); }); // Only a re-renders @@ -258,11 +256,13 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A1B1'); }); + // In React 18, you can't observe in between a sync render and its + // passive effects, so this is only relevant to legacy roots // @gate !variant test( "compares to current state before bailing out, even when there's a " + 'mutation in between the sync and passive effects', - () => { + async () => { const store = createExternalStore(0); function App() { @@ -302,7 +302,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { }, ); - // @gate !variant test('mutating the store in between render and commit when getSnapshot has changed', () => { const store = createExternalStore({a: 1, b: 1}); @@ -362,7 +361,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('B2'); }); - // @gate !variant test('mutating the store in between render and commit when getSnapshot has _not_ changed', () => { // Same as previous test, but `getSnapshot` does not change const store = createExternalStore({a: 1, b: 1}); @@ -421,8 +419,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A1'); }); - // @gate !variant - test("does not bail out if the previous update hasn't finished yet", () => { + test("does not bail out if the previous update hasn't finished yet", async () => { const store = createExternalStore(0); function Child1() { @@ -453,14 +450,13 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(Scheduler).toHaveYielded([0, 0]); expect(root).toMatchRenderedOutput('00'); - act(() => { + await act(() => { store.set(1); }); expect(Scheduler).toHaveYielded([1, 1, 'Reset back to 0', 0, 0]); expect(root).toMatchRenderedOutput('00'); }); - // @gate !variant test('uses the latest getSnapshot, even if it changed in the same batch as a store update', () => { const store = createExternalStore({a: 0, b: 0}); @@ -481,16 +477,17 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { // Update the store and getSnapshot at the same time act(() => { - setGetSnapshot(() => getSnapshotB); - store.set({a: 1, b: 2}); + ReactNoop.flushSync(() => { + setGetSnapshot(() => getSnapshotB); + store.set({a: 1, b: 2}); + }); }); // It should read from B instead of A expect(Scheduler).toHaveYielded([2]); expect(root).toMatchRenderedOutput('2'); }); - // @gate !variant - test('handles errors thrown by getSnapshot or isEqual', () => { + test('handles errors thrown by getSnapshot', async () => { class ErrorBoundary extends React.Component { state = {error: null}; static getDerivedStateFromError(error) { @@ -511,24 +508,13 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { }); function App() { - const {value} = useSyncExternalStore( - store.subscribe, - () => { - const state = store.getState(); - if (state.throwInGetSnapshot) { - throw new Error('Error in getSnapshot'); - } - return state; - }, - { - isEqual: (a, b) => { - if (a.throwInIsEqual || b.throwInIsEqual) { - throw new Error('Error in isEqual'); - } - return a.value === b.value; - }, - }, - ); + const {value} = useSyncExternalStore(store.subscribe, () => { + const state = store.getState(); + if (state.throwInGetSnapshot) { + throw new Error('Error in getSnapshot'); + } + return state; + }); return ; } @@ -545,30 +531,45 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('0'); // Update that throws in a getSnapshot. We can catch it with an error boundary. - act(() => { + await act(() => { store.set({value: 1, throwInGetSnapshot: true, throwInIsEqual: false}); }); - expect(Scheduler).toHaveYielded(['Error in getSnapshot']); + if (gate(flags => flags.variant)) { + expect(Scheduler).toHaveYielded([ + 'Error in getSnapshot', + // In a concurrent root, React renders a second time to attempt to + // recover from the error. + 'Error in getSnapshot', + ]); + } else { + expect(Scheduler).toHaveYielded(['Error in getSnapshot']); + } expect(root).toMatchRenderedOutput('Error in getSnapshot'); + }); - // Clear the error. - act(() => { - store.set({value: 1, throwInGetSnapshot: false, throwInIsEqual: false}); - errorBoundary.current.setState({error: null}); - }); - expect(Scheduler).toHaveYielded([1]); - expect(root).toMatchRenderedOutput('1'); + test('Infinite loop if getSnapshot keeps returning new reference', () => { + const store = createExternalStore({}); - // Update that throws in isEqual. Since isEqual only prevents a bail out, - // we don't need to surface an error. But we do have to re-render. - act(() => { - store.set({value: 1, throwInGetSnapshot: false, throwInIsEqual: true}); - }); - expect(Scheduler).toHaveYielded([1]); - expect(root).toMatchRenderedOutput('1'); + function App() { + const text = useSyncExternalStore(store.subscribe, () => ({})); + return ; + } + + spyOnDev(console, 'error'); + const root = createRoot(); + + expect(() => act(() => root.render())).toThrow( + 'Maximum update depth exceeded. This can happen when a component repeatedly ' + + 'calls setState inside componentWillUpdate or componentDidUpdate. React limits ' + + 'the number of nested updates to prevent infinite loops.', + ); + if (__DEV__) { + expect(console.error.calls.argsFor(0)[0]).toMatch( + 'The result of getSnapshot should be cached to avoid an infinite loop', + ); + } }); - // @gate !variant test('Infinite loop if getSnapshot keeps returning new reference', () => { const store = createExternalStore({}); @@ -593,8 +594,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { }); describe('extra features implemented in user-space', () => { - // @gate !variant - test('memoized selectors are only called once per update', () => { + test('memoized selectors are only called once per update', async () => { const store = createExternalStore({a: 0, b: 0}); function selector(state) { @@ -619,7 +619,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A0'); // Update the store - act(() => { + await act(() => { store.set({a: 1, b: 0}); }); expect(Scheduler).toHaveYielded([ @@ -633,8 +633,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A1'); }); - // @gate !variant - test('Using isEqual to bailout', () => { + test('Using isEqual to bailout', async () => { const store = createExternalStore({a: 0, b: 0}); function A() { @@ -674,7 +673,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A0B0'); // Update b but not a - act(() => { + await act(() => { store.set({a: 0, b: 1}); }); // Only b re-renders @@ -682,7 +681,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A0B1'); // Update a but not b - act(() => { + await act(() => { store.set({a: 1, b: 1}); }); // Only a re-renders