diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 8eefb9dc2f0a3..72946204f53e0 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -30,6 +30,7 @@ import type { import ReactSharedInternals from 'shared/ReactSharedInternals'; import {enableUseEventAPI} from 'shared/ReactFeatureFlags'; +import {markRootExpiredAtTime} from './ReactFiberRoot'; import {NoWork, Sync} from './ReactFiberExpirationTime'; import {readContext} from './ReactFiberNewContext'; import {createDeprecatedResponderListener} from './ReactFiberDeprecatedEvents'; @@ -885,6 +886,7 @@ function rerenderReducer( type MutableSourceMemoizedState = {| refs: { getSnapshot: MutableSourceGetSnapshotFn, + setSnapshot: Snapshot => void, }, source: MutableSource, subscribe: MutableSourceSubscribeFn, @@ -998,7 +1000,40 @@ function useMutableSource( // Sync the values needed by our subscribe function after each commit. dispatcher.useEffect(() => { + const didGetSnapshotChange = !is(refs.getSnapshot, getSnapshot); refs.getSnapshot = getSnapshot; + + // Normally the dispatch function for a state hook never changes, + // but in the case of this hook, it will change if getSnapshot changes. + // In that case, the subscription below will have cloesd over the previous function, + // so we use a ref to ensure that handleChange() always has the latest version. + refs.setSnapshot = setSnapshot; + + // This effect runs on mount, even though getSnapshot hasn't changed. + // In that case we can avoid the additional checks for a changed snapshot, + // because the subscription effect below will cover this. + if (didGetSnapshotChange) { + // Because getSnapshot is shared with subscriptions via a ref, + // we don't resubscribe when getSnapshot changes. + // This means that we also don't check for any missed mutations + // between the render and the passive commit though. + // So we need to check here, just like when we newly subscribe. + const maybeNewVersion = getVersion(source._source); + if (!is(version, maybeNewVersion)) { + const maybeNewSnapshot = getSnapshot(source._source); + if (!is(snapshot, maybeNewSnapshot)) { + setSnapshot(maybeNewSnapshot); + + // If the source mutated between render and now, + // there may be state updates already scheduled from the old getSnapshot. + // Those updates should not commit without this value. + // There is no mechanism currently to associate these updates though, + // so for now we fall back to synchronously flushing all pending updates. + // TODO: Improve this later. + markRootExpiredAtTime(root, getPendingExpirationTime(root)); + } + } + } }, [getSnapshot]); // If we got a new source or subscribe function, @@ -1007,8 +1042,10 @@ function useMutableSource( dispatcher.useEffect(() => { const handleChange = () => { const latestGetSnapshot = refs.getSnapshot; + const latestSetSnapshot = refs.setSnapshot; + try { - setSnapshot(latestGetSnapshot(source._source)); + latestSetSnapshot(latestGetSnapshot(source._source)); // Record a pending mutable source update with the same expiration time. const currentTime = requestCurrentTimeForUpdate(); @@ -1025,9 +1062,11 @@ function useMutableSource( // e.g. it might try to read from a part of the store that no longer exists. // In this case we should still schedule an update with React. // Worst case the selector will throw again and then an error boundary will handle it. - setSnapshot(() => { - throw error; - }); + latestSetSnapshot( + (() => { + throw error; + }: any), + ); } }; @@ -1046,6 +1085,11 @@ function useMutableSource( const maybeNewSnapshot = getSnapshot(source._source); if (!is(snapshot, maybeNewSnapshot)) { setSnapshot(maybeNewSnapshot); + + // We missed a mutation before committing. + // It's possible that other components using this source also have pending updates scheduled. + // In that case, we should ensure they all commit together. + markRootExpiredAtTime(root, getPendingExpirationTime(root)); } } @@ -1063,11 +1107,31 @@ function useMutableSource( // // In both cases, we need to throw away pending udpates (since they are no longer relevant) // and treat reading from the source as we do in the mount case. + const didGetSnapshotChange = !is(prevGetSnapshot, getSnapshot); if ( !is(prevSource, source) || !is(prevSubscribe, subscribe) || - !is(prevGetSnapshot, getSnapshot) + didGetSnapshotChange ) { + if (didGetSnapshotChange) { + // Create a new queue and setState method, + // So if there are interleaved updates, they get pushed to the older queue. + // When this becomes current, the previous queue and dispatch method will be discarded, + // including any interleaving updates that occur. + const newQueue = { + pending: null, + dispatch: null, + lastRenderedReducer: basicStateReducer, + lastRenderedState: snapshot, + }; + newQueue.dispatch = setSnapshot = (dispatchAction.bind( + null, + currentlyRenderingFiber, + newQueue, + ): any); + stateHook.queue = newQueue; + } + stateHook.baseQueue = null; snapshot = readFromUnsubcribedMutableSource(root, source, getSnapshot); stateHook.memoizedState = stateHook.baseState = snapshot; @@ -1085,6 +1149,7 @@ function mountMutableSource( hook.memoizedState = ({ refs: { getSnapshot, + setSnapshot: (null: any), }, source, subscribe, diff --git a/packages/react-reconciler/src/ReactMutableSource.js b/packages/react-reconciler/src/ReactMutableSource.js index e0cf6857d8724..b5e343fb15954 100644 --- a/packages/react-reconciler/src/ReactMutableSource.js +++ b/packages/react-reconciler/src/ReactMutableSource.js @@ -43,7 +43,16 @@ export function setPendingExpirationTime( root: FiberRoot, expirationTime: ExpirationTime, ): void { - root.mutableSourcePendingUpdateTime = expirationTime; + // Because we only track one pending update time per root, + // track the lowest priority update. + // It's inclusive of all other pending updates. + // + // TODO This currently gives us a false positive in some cases + // when it comes to determining if it's safe to read during an update. + root.mutableSourcePendingUpdateTime = Math.max( + root.mutableSourcePendingUpdateTime, + expirationTime, + ); } export function markSourceAsDirty(mutableSource: MutableSource): void { diff --git a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js index 3d875ffc89dc4..9a46c61d97cdc 100644 --- a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js +++ b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js @@ -43,8 +43,8 @@ describe('useMutableSource', () => { const callbacksA = []; const callbacksB = []; let revision = 0; - let valueA = 'a:one'; - let valueB = 'b:one'; + let valueA = initialValueA; + let valueB = initialValueB; const subscribeHelper = (callbacks, callback) => { if (callbacks.indexOf(callback) < 0) { @@ -630,7 +630,7 @@ describe('useMutableSource', () => { }); it('should only update components whose subscriptions fire', () => { - const source = createComplexSource('one', 'one'); + const source = createComplexSource('a:one', 'b:one'); const mutableSource = createMutableSource(source); // Subscribe to part of the store. @@ -672,7 +672,7 @@ describe('useMutableSource', () => { }); it('should detect tearing in part of the store not yet subscribed to', () => { - const source = createComplexSource('one', 'one'); + const source = createComplexSource('a:one', 'b:one'); const mutableSource = createMutableSource(source); // Subscribe to part of the store. @@ -1054,32 +1054,31 @@ describe('useMutableSource', () => { }; } - function App({toggle}) { + function App({getSnapshot}) { const state = useMutableSource( mutableSource, - toggle ? getSnapshotB : getSnapshotA, + getSnapshot, defaultSubscribe, ); - const result = (toggle ? 'on: ' : 'off: ') + state; - return result; + return state; } const root = ReactNoop.createRoot(); await act(async () => { - root.render(); + root.render(); }); - expect(root).toMatchRenderedOutput('off: initial'); + expect(root).toMatchRenderedOutput('initial'); await act(async () => { mutateB('Updated B'); - root.render(); + root.render(); }); - expect(root).toMatchRenderedOutput('on: Updated B'); + expect(root).toMatchRenderedOutput('Updated B'); await act(async () => { mutateB('Another update'); }); - expect(root).toMatchRenderedOutput('on: Another update'); + expect(root).toMatchRenderedOutput('Another update'); }); it('should clear the update queue when getSnapshot changes with pending lower priority updates', async () => { @@ -1265,6 +1264,325 @@ describe('useMutableSource', () => { expect(Scheduler).toHaveYielded(['x: bar, y: bar']); }); + it('getSnapshot changes and then source is mutated in between paint and passive effect phase', async () => { + const source = createSource({ + a: 'foo', + b: 'bar', + }); + const mutableSource = createMutableSource(source); + + function mutateB(newB) { + source.value = { + ...source.value, + b: newB, + }; + } + + const getSnapshotA = () => source.value.a; + const getSnapshotB = () => source.value.b; + + function App({getSnapshot}) { + const value = useMutableSource( + mutableSource, + getSnapshot, + defaultSubscribe, + ); + + Scheduler.unstable_yieldValue('Render: ' + value); + React.useEffect(() => { + Scheduler.unstable_yieldValue('Commit: ' + value); + }, [value]); + + return value; + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Render: foo', 'Commit: foo']); + + await act(async () => { + // Switch getSnapshot to read from B instead + root.render(); + // Render and finish the tree, but yield right after paint, before + // the passive effects have fired. + expect(Scheduler).toFlushUntilNextPaint(['Render: bar']); + // Then mutate B. + mutateB('baz'); + }); + expect(Scheduler).toHaveYielded([ + // Fires the effect from the previous render + 'Commit: bar', + // During that effect, it should detect that the snapshot has changed + // and re-render. + 'Render: baz', + 'Commit: baz', + ]); + expect(root).toMatchRenderedOutput('baz'); + }); + + it('getSnapshot changes and then source is mutated in between paint and passive effect phase, case 2', async () => { + const source = createSource({ + a: 'a0', + b: 'b0', + }); + const mutableSource = createMutableSource(source); + + const getSnapshotA = () => source.value.a; + const getSnapshotB = () => source.value.b; + + function mutateA(newA) { + source.value = { + ...source.value, + a: newA, + }; + } + + function App({getSnapshotFirst, getSnapshotSecond}) { + const first = useMutableSource( + mutableSource, + getSnapshotFirst, + defaultSubscribe, + ); + const second = useMutableSource( + mutableSource, + getSnapshotSecond, + defaultSubscribe, + ); + + return `first: ${first}, second: ${second}`; + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render( + , + ); + }); + expect(root.getChildrenAsJSX()).toEqual('first: a0, second: b0'); + + await act(async () => { + // Switch the second getSnapshot to also read from A + root.render( + , + ); + // Render and finish the tree, but yield right after paint, before + // the passive effects have fired. + expect(Scheduler).toFlushUntilNextPaint([]); + + // Now mutate A. Both hooks should update. + // This is at high priority so that it doesn't get batched with default + // priority updates that might fire during the passive effect + ReactNoop.discreteUpdates(() => { + mutateA('a1'); + }); + expect(Scheduler).toFlushUntilNextPaint([]); + + // TODO: This test currently tears. + // Fill in with correct values once the entanglement issue has been fixed. + expect(root.getChildrenAsJSX()).toEqual('first: a1, second: a0'); + }); + + expect(root.getChildrenAsJSX()).toEqual('first: a1, second: a1'); + }); + + it('getSnapshot changes and then source is mutated during interleaved event', async () => { + const {useEffect} = React; + + const source = createComplexSource('1', '2'); + const mutableSource = createMutableSource(source); + + // Subscribe to part of the store. + const getSnapshotA = s => s.valueA; + const subscribeA = (s, callback) => s.subscribeA(callback); + const configA = [getSnapshotA, subscribeA]; + + const getSnapshotB = s => s.valueB; + const subscribeB = (s, callback) => s.subscribeB(callback); + const configB = [getSnapshotB, subscribeB]; + + function App({parentConfig, childConfig}) { + const [getSnapshot, subscribe] = parentConfig; + const parentValue = useMutableSource( + mutableSource, + getSnapshot, + subscribe, + ); + + Scheduler.unstable_yieldValue('Parent: ' + parentValue); + + return ( + + ); + } + + function Child({parentConfig, childConfig, parentValue}) { + const [getSnapshot, subscribe] = childConfig; + const childValue = useMutableSource( + mutableSource, + getSnapshot, + subscribe, + ); + + Scheduler.unstable_yieldValue('Child: ' + childValue); + + let result = `${parentValue}, ${childValue}`; + + if (parentConfig === childConfig) { + // When both components read using the same config, the two values + // must be consistent. + if (parentValue !== childValue) { + result = 'Oops, tearing!'; + } + } + + useEffect(() => { + Scheduler.unstable_yieldValue('Commit: ' + result); + }, [result]); + + return result; + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded([ + 'Parent: 1', + 'Child: 2', + 'Commit: 1, 2', + ]); + + await act(async () => { + // Switch the parent and the child to read using the same config + root.render(); + // Start rendering the parent, but yield before rendering the child + expect(Scheduler).toFlushAndYieldThrough(['Parent: 2']); + + // Mutate the config. This is at lower priority so that 1) to make sure + // it doesn't happen to get batched with the in-progress render, and 2) + // so it doesn't interrupt the in-progress render. + Scheduler.unstable_runWithPriority( + Scheduler.unstable_IdlePriority, + () => { + source.valueB = '3'; + }, + ); + }); + + // TODO: This test currently tears. + // Fill in with correct values once the entanglement issue has been fixed. + // Here's the current behavior: + expect(Scheduler).toHaveYielded([ + // The partial render completes + 'Child: 2', + 'Commit: 2, 2', + + // Then we start rendering the low priority mutation + 'Parent: 3', + // But the child never received a mutation event, because it hadn't + // mounted yet. So the render tears. + 'Child: 2', + 'Commit: Oops, tearing!', + + // Eventually the child corrects itself, because of the check that + // occurs when re-subscribing. + 'Child: 3', + 'Commit: 3, 3', + ]); + }); + + it('should not tear with newly mounted component when updates were scheduled at a lower priority', async () => { + const source = createSource('one'); + const mutableSource = createMutableSource(source); + + let committedA = null; + let committedB = null; + + const onRender = () => { + if (committedB !== null) { + expect(committedA).toBe(committedB); + } + }; + + function ComponentA() { + const snapshot = useMutableSource( + mutableSource, + defaultGetSnapshot, + defaultSubscribe, + ); + Scheduler.unstable_yieldValue(`a:${snapshot}`); + React.useEffect(() => { + committedA = snapshot; + }, [snapshot]); + return
{`a:${snapshot}`}
; + } + function ComponentB() { + const snapshot = useMutableSource( + mutableSource, + defaultGetSnapshot, + defaultSubscribe, + ); + Scheduler.unstable_yieldValue(`b:${snapshot}`); + React.useEffect(() => { + committedB = snapshot; + }, [snapshot]); + return
{`b:${snapshot}`}
; + } + + // Mount ComponentA with data version 1 + act(() => { + ReactNoop.render( + + + , + () => Scheduler.unstable_yieldValue('Sync effect'), + ); + }); + expect(Scheduler).toHaveYielded(['a:one', 'Sync effect']); + expect(source.listenerCount).toBe(1); + + // Mount ComponentB with version 1 (but don't commit it) + act(() => { + ReactNoop.render( + + + + , + () => Scheduler.unstable_yieldValue('Sync effect'), + ); + expect(Scheduler).toFlushAndYieldThrough([ + 'a:one', + 'b:one', + 'Sync effect', + ]); + expect(source.listenerCount).toBe(1); + + // Mutate -> schedule update for ComponentA + Scheduler.unstable_runWithPriority( + Scheduler.unstable_IdlePriority, + () => { + source.value = 'two'; + }, + ); + + // Commit ComponentB -> notice the change and schedule an update for ComponentB + expect(Scheduler).toFlushAndYield(['a:two', 'b:two']); + expect(source.listenerCount).toBe(2); + }); + }); + if (__DEV__) { describe('dev warnings', () => { it('should warn if the subscribe function does not return an unsubscribe function', () => {