From 9bee11dcfe2e196fd7c674b42829a624b448e6c0 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 31 Mar 2020 15:20:46 -0700 Subject: [PATCH 1/5] useMutableSource: bugfix for new getSnapshot with mutation --- .../react-reconciler/src/ReactFiberHooks.js | 75 +++- .../src/ReactMutableSource.js | 11 +- .../useMutableSource-test.internal.js | 344 +++++++++++++++++- 3 files changed, 411 insertions(+), 19 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index c511634a8f36..bf10a0f259da 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -31,6 +31,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'; @@ -886,6 +887,7 @@ function rerenderReducer( type MutableSourceMemoizedState = {| refs: { getSnapshot: MutableSourceGetSnapshotFn, + setSnapshot: Snapshot => void, }, source: MutableSource, subscribe: MutableSourceSubscribeFn, @@ -1000,7 +1002,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, @@ -1009,8 +1044,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(); @@ -1027,9 +1064,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), + ); } }; @@ -1048,6 +1087,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)); } } @@ -1065,11 +1109,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; @@ -1087,6 +1151,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 37c032201a07..1afad5bbc905 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 3d875ffc89dc..9a46c61d97cd 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', () => { From 4d25d4e335f38cc88d167a400b95ea47558df627 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 2 Apr 2020 09:39:02 -0700 Subject: [PATCH 2/5] Split mutable source pending expiration times into first and last --- .../react-reconciler/src/ReactFiberHooks.js | 16 +++++----- .../react-reconciler/src/ReactFiberRoot.js | 6 ++-- .../src/ReactMutableSource.js | 31 +++++++++++++------ .../useMutableSource-test.internal.js | 4 +-- 4 files changed, 36 insertions(+), 21 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index bf10a0f259da..db0da30e7d2f 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -75,7 +75,8 @@ import { getCurrentPriorityLevel, } from './SchedulerWithReactIntegration'; import { - getPendingExpirationTime, + getFirstPendingExpirationTime, + getLastPendingExpirationTime, getWorkInProgressVersion, markSourceAsDirty, setPendingExpirationTime, @@ -916,16 +917,14 @@ function readFromUnsubcribedMutableSource( isSafeToReadFromSource = currentRenderVersion === version; } else { // If there's no version, then we should fallback to checking the update time. - const pendingExpirationTime = getPendingExpirationTime(root); + const pendingExpirationTime = getFirstPendingExpirationTime(root); if (pendingExpirationTime === NoWork) { isSafeToReadFromSource = true; } else { // If the source has pending updates, we can use the current render's expiration // time to determine if it's safe to read again from the source. - isSafeToReadFromSource = - pendingExpirationTime === NoWork || - pendingExpirationTime >= renderExpirationTime; + isSafeToReadFromSource = pendingExpirationTime >= renderExpirationTime; } if (isSafeToReadFromSource) { @@ -974,7 +973,8 @@ function useMutableSource( const dispatcher = ReactCurrentDispatcher.current; - const [currentSnapshot, setSnapshot] = dispatcher.useState(() => + // eslint-disable-next-line prefer-const + let [currentSnapshot, setSnapshot] = dispatcher.useState(() => readFromUnsubcribedMutableSource(root, source, getSnapshot), ); let snapshot = currentSnapshot; @@ -1032,7 +1032,7 @@ function useMutableSource( // 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)); + markRootExpiredAtTime(root, getLastPendingExpirationTime(root)); } } } @@ -1091,7 +1091,7 @@ function useMutableSource( // 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)); + markRootExpiredAtTime(root, getLastPendingExpirationTime(root)); } } diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index 544dcd076414..767ed8aaca8d 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -79,7 +79,8 @@ type BaseFiberRootProperties = {| lastExpiredTime: ExpirationTime, // Used by useMutableSource hook to avoid tearing within this root // when external, mutable sources are read from during render. - mutableSourcePendingUpdateTime: ExpirationTime, + mutableSourceFirstPendingUpdateTime: ExpirationTime, + mutableSourceLastPendingUpdateTime: ExpirationTime, |}; // The following attributes are only used by interaction tracing builds. @@ -130,7 +131,8 @@ function FiberRootNode(containerInfo, tag, hydrate) { this.nextKnownPendingLevel = NoWork; this.lastPingedTime = NoWork; this.lastExpiredTime = NoWork; - this.mutableSourcePendingUpdateTime = NoWork; + this.mutableSourceFirstPendingUpdateTime = NoWork; + this.mutableSourceLastPendingUpdateTime = NoWork; if (enableSchedulerTracing) { this.interactionThreadID = unstable_getThreadID(); diff --git a/packages/react-reconciler/src/ReactMutableSource.js b/packages/react-reconciler/src/ReactMutableSource.js index 1afad5bbc905..99a2c612f425 100644 --- a/packages/react-reconciler/src/ReactMutableSource.js +++ b/packages/react-reconciler/src/ReactMutableSource.js @@ -30,13 +30,20 @@ export function clearPendingUpdates( root: FiberRoot, expirationTime: ExpirationTime, ): void { - if (root.mutableSourcePendingUpdateTime <= expirationTime) { - root.mutableSourcePendingUpdateTime = NoWork; + if (root.mutableSourceFirstPendingUpdateTime <= expirationTime) { + root.mutableSourceFirstPendingUpdateTime = NoWork; } + if (root.mutableSourceLastPendingUpdateTime <= expirationTime) { + root.mutableSourceLastPendingUpdateTime = NoWork; + } +} + +export function getFirstPendingExpirationTime(root: FiberRoot): ExpirationTime { + return root.mutableSourceFirstPendingUpdateTime; } -export function getPendingExpirationTime(root: FiberRoot): ExpirationTime { - return root.mutableSourcePendingUpdateTime; +export function getLastPendingExpirationTime(root: FiberRoot): ExpirationTime { + return root.mutableSourceLastPendingUpdateTime; } export function setPendingExpirationTime( @@ -46,13 +53,19 @@ export function setPendingExpirationTime( // 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, + root.mutableSourceLastPendingUpdateTime = Math.max( + root.mutableSourceLastPendingUpdateTime, expirationTime, ); + + if (root.mutableSourceFirstPendingUpdateTime === NoWork) { + root.mutableSourceFirstPendingUpdateTime = expirationTime; + } else { + root.mutableSourceFirstPendingUpdateTime = Math.min( + root.mutableSourceFirstPendingUpdateTime, + 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 9a46c61d97cd..114dbb19a000 100644 --- a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js +++ b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js @@ -1237,7 +1237,7 @@ describe('useMutableSource', () => { ReactNoop.discreteUpdates(() => { // At high priority, toggle y so that it reads from A instead of B. // Simultaneously, mutate A. - mutateA('high-pri baz'); + mutateA('baz'); root.render( { ); // If this update were processed before the next mutation, - // it would be expected to yield "high-pri baz" and "high-pri baz". + // it would be expected to yield "baz" and "baz". }); // At lower priority, mutate A again. From 1a73af5902c4bb11174dbd1e9e1e18fe606bfd14 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 2 Apr 2020 15:21:53 -0700 Subject: [PATCH 3/5] Fixed remaining tearing cases --- .../react-reconciler/src/ReactFiberHooks.js | 23 ++++++++++++++++--- .../src/ReactMutableSource.js | 23 +++++++++---------- .../useMutableSource-test.internal.js | 11 +-------- 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index db0da30e7d2f..a37641245d0e 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -76,7 +76,6 @@ import { } from './SchedulerWithReactIntegration'; import { getFirstPendingExpirationTime, - getLastPendingExpirationTime, getWorkInProgressVersion, markSourceAsDirty, setPendingExpirationTime, @@ -1026,13 +1025,22 @@ function useMutableSource( if (!is(snapshot, maybeNewSnapshot)) { setSnapshot(maybeNewSnapshot); + const currentTime = requestCurrentTimeForUpdate(); + const suspenseConfig = requestCurrentSuspenseConfig(); + const expirationTime = computeExpirationForFiber( + currentTime, + fiber, + suspenseConfig, + ); + setPendingExpirationTime(root, expirationTime); + // 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, getLastPendingExpirationTime(root)); + markRootExpiredAtTime(root, getFirstPendingExpirationTime(root)); } } } @@ -1088,10 +1096,19 @@ function useMutableSource( if (!is(snapshot, maybeNewSnapshot)) { setSnapshot(maybeNewSnapshot); + const currentTime = requestCurrentTimeForUpdate(); + const suspenseConfig = requestCurrentSuspenseConfig(); + const expirationTime = computeExpirationForFiber( + currentTime, + fiber, + suspenseConfig, + ); + setPendingExpirationTime(root, expirationTime); + // 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, getLastPendingExpirationTime(root)); + markRootExpiredAtTime(root, getFirstPendingExpirationTime(root)); } } diff --git a/packages/react-reconciler/src/ReactMutableSource.js b/packages/react-reconciler/src/ReactMutableSource.js index 99a2c612f425..ead89e52f37f 100644 --- a/packages/react-reconciler/src/ReactMutableSource.js +++ b/packages/react-reconciler/src/ReactMutableSource.js @@ -30,11 +30,14 @@ export function clearPendingUpdates( root: FiberRoot, expirationTime: ExpirationTime, ): void { - if (root.mutableSourceFirstPendingUpdateTime <= expirationTime) { + if (expirationTime <= root.mutableSourceLastPendingUpdateTime) { + // All updates for this source have been processed. root.mutableSourceFirstPendingUpdateTime = NoWork; - } - if (root.mutableSourceLastPendingUpdateTime <= expirationTime) { root.mutableSourceLastPendingUpdateTime = NoWork; + } else if (expirationTime <= root.mutableSourceFirstPendingUpdateTime) { + // The highest priority pending updates have been processed, + // but we don't how many updates remain between the current and lowest priority. + root.mutableSourceFirstPendingUpdateTime = expirationTime - 1; } } @@ -53,18 +56,14 @@ export function setPendingExpirationTime( // Because we only track one pending update time per root, // track the lowest priority update. // It's inclusive of all other pending updates. - root.mutableSourceLastPendingUpdateTime = Math.max( - root.mutableSourceLastPendingUpdateTime, - expirationTime, - ); + if (expirationTime > root.mutableSourceLastPendingUpdateTime) { + root.mutableSourceLastPendingUpdateTime = expirationTime; + } if (root.mutableSourceFirstPendingUpdateTime === NoWork) { root.mutableSourceFirstPendingUpdateTime = expirationTime; - } else { - root.mutableSourceFirstPendingUpdateTime = Math.min( - root.mutableSourceFirstPendingUpdateTime, - expirationTime, - ); + } else if (expirationTime < root.mutableSourceFirstPendingUpdateTime) { + root.mutableSourceFirstPendingUpdateTime = expirationTime; } } diff --git a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js index 114dbb19a000..61d753ddf1c9 100644 --- a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js +++ b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js @@ -1385,9 +1385,7 @@ describe('useMutableSource', () => { }); 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'); }); expect(root.getChildrenAsJSX()).toEqual('first: a1, second: a1'); @@ -1481,9 +1479,6 @@ describe('useMutableSource', () => { ); }); - // 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', @@ -1491,10 +1486,6 @@ describe('useMutableSource', () => { // 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. From db511f4fc640e46a2bcbb3888080b9182bed04c4 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 3 Apr 2020 09:04:07 -0700 Subject: [PATCH 4/5] Swapped first/last logic in setPendingExpirationTime and removed no longer in use "first" time value --- .../react-reconciler/src/ReactFiberHooks.js | 8 +++--- .../react-reconciler/src/ReactFiberRoot.js | 1 - .../src/ReactMutableSource.js | 25 +++++-------------- 3 files changed, 10 insertions(+), 24 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index a37641245d0e..8875323596ce 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -75,7 +75,7 @@ import { getCurrentPriorityLevel, } from './SchedulerWithReactIntegration'; import { - getFirstPendingExpirationTime, + getLastPendingExpirationTime, getWorkInProgressVersion, markSourceAsDirty, setPendingExpirationTime, @@ -916,7 +916,7 @@ function readFromUnsubcribedMutableSource( isSafeToReadFromSource = currentRenderVersion === version; } else { // If there's no version, then we should fallback to checking the update time. - const pendingExpirationTime = getFirstPendingExpirationTime(root); + const pendingExpirationTime = getLastPendingExpirationTime(root); if (pendingExpirationTime === NoWork) { isSafeToReadFromSource = true; @@ -1040,7 +1040,7 @@ function useMutableSource( // 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, getFirstPendingExpirationTime(root)); + markRootExpiredAtTime(root, getLastPendingExpirationTime(root)); } } } @@ -1108,7 +1108,7 @@ function useMutableSource( // 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, getFirstPendingExpirationTime(root)); + markRootExpiredAtTime(root, getLastPendingExpirationTime(root)); } } diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index 767ed8aaca8d..26b93bd2018f 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -79,7 +79,6 @@ type BaseFiberRootProperties = {| lastExpiredTime: ExpirationTime, // Used by useMutableSource hook to avoid tearing within this root // when external, mutable sources are read from during render. - mutableSourceFirstPendingUpdateTime: ExpirationTime, mutableSourceLastPendingUpdateTime: ExpirationTime, |}; diff --git a/packages/react-reconciler/src/ReactMutableSource.js b/packages/react-reconciler/src/ReactMutableSource.js index ead89e52f37f..1d6ade4a6722 100644 --- a/packages/react-reconciler/src/ReactMutableSource.js +++ b/packages/react-reconciler/src/ReactMutableSource.js @@ -32,19 +32,10 @@ export function clearPendingUpdates( ): void { if (expirationTime <= root.mutableSourceLastPendingUpdateTime) { // All updates for this source have been processed. - root.mutableSourceFirstPendingUpdateTime = NoWork; root.mutableSourceLastPendingUpdateTime = NoWork; - } else if (expirationTime <= root.mutableSourceFirstPendingUpdateTime) { - // The highest priority pending updates have been processed, - // but we don't how many updates remain between the current and lowest priority. - root.mutableSourceFirstPendingUpdateTime = expirationTime - 1; } } -export function getFirstPendingExpirationTime(root: FiberRoot): ExpirationTime { - return root.mutableSourceFirstPendingUpdateTime; -} - export function getLastPendingExpirationTime(root: FiberRoot): ExpirationTime { return root.mutableSourceLastPendingUpdateTime; } @@ -53,18 +44,14 @@ export function setPendingExpirationTime( root: FiberRoot, expirationTime: ExpirationTime, ): void { - // Because we only track one pending update time per root, - // track the lowest priority update. - // It's inclusive of all other pending updates. - if (expirationTime > root.mutableSourceLastPendingUpdateTime) { + const mutableSourceLastPendingUpdateTime = + root.mutableSourceLastPendingUpdateTime; + if ( + mutableSourceLastPendingUpdateTime === NoWork || + expirationTime < mutableSourceLastPendingUpdateTime + ) { root.mutableSourceLastPendingUpdateTime = expirationTime; } - - if (root.mutableSourceFirstPendingUpdateTime === NoWork) { - root.mutableSourceFirstPendingUpdateTime = expirationTime; - } else if (expirationTime < root.mutableSourceFirstPendingUpdateTime) { - root.mutableSourceFirstPendingUpdateTime = expirationTime; - } } export function markSourceAsDirty(mutableSource: MutableSource): void { From 86da5ce041477efb70aa7b4d70df53f001a985bb Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 3 Apr 2020 09:59:55 -0700 Subject: [PATCH 5/5] Recreate queue and dispatch when source/subscribe/getSnapshot change --- .../react-reconciler/src/ReactFiberHooks.js | 133 +++++++----------- .../useMutableSource-test.internal.js | 2 +- 2 files changed, 48 insertions(+), 87 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 8875323596ce..580311570542 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -999,56 +999,44 @@ function useMutableSource( subscribe, }: MutableSourceMemoizedState); - // Sync the values needed by our subscribe function after each commit. + // Sync the values needed by our subscription handler 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. + // but this hook recreates the queue in certain cases to avoid updates from stale sources. + // handleChange() below needs to reference the dispatch function without re-subscribing, + // so we use a ref to ensure that it 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); - - const currentTime = requestCurrentTimeForUpdate(); - const suspenseConfig = requestCurrentSuspenseConfig(); - const expirationTime = computeExpirationForFiber( - currentTime, - fiber, - suspenseConfig, - ); - setPendingExpirationTime(root, expirationTime); - - // 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, getLastPendingExpirationTime(root)); - } + // Check for a possible change between when we last rendered now. + const maybeNewVersion = getVersion(source._source); + if (!is(version, maybeNewVersion)) { + const maybeNewSnapshot = getSnapshot(source._source); + if (!is(snapshot, maybeNewSnapshot)) { + setSnapshot(maybeNewSnapshot); + + const currentTime = requestCurrentTimeForUpdate(); + const suspenseConfig = requestCurrentSuspenseConfig(); + const expirationTime = computeExpirationForFiber( + currentTime, + fiber, + suspenseConfig, + ); + setPendingExpirationTime(root, expirationTime); + + // 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, getLastPendingExpirationTime(root)); } } - }, [getSnapshot]); + }, [getSnapshot, source, subscribe]); - // If we got a new source or subscribe function, - // we'll need to subscribe in a passive effect, - // and also check for any changes that fire between render and subscribe. + // If we got a new source or subscribe function, re-subscribe in a passive effect. dispatcher.useEffect(() => { const handleChange = () => { const latestGetSnapshot = refs.getSnapshot; @@ -1089,29 +1077,6 @@ function useMutableSource( } } - // Check for a possible change between when we last rendered and when we just subscribed. - const maybeNewVersion = getVersion(source._source); - if (!is(version, maybeNewVersion)) { - const maybeNewSnapshot = getSnapshot(source._source); - if (!is(snapshot, maybeNewSnapshot)) { - setSnapshot(maybeNewSnapshot); - - const currentTime = requestCurrentTimeForUpdate(); - const suspenseConfig = requestCurrentSuspenseConfig(); - const expirationTime = computeExpirationForFiber( - currentTime, - fiber, - suspenseConfig, - ); - setPendingExpirationTime(root, expirationTime); - - // 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, getLastPendingExpirationTime(root)); - } - } - return unsubscribe; }, [source, subscribe]); @@ -1126,31 +1091,27 @@ 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(prevGetSnapshot, getSnapshot) || !is(prevSource, source) || - !is(prevSubscribe, subscribe) || - didGetSnapshotChange + !is(prevSubscribe, subscribe) ) { - 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; - } - + // 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; diff --git a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js index 61d753ddf1c9..d9ff4ad3ac2d 100644 --- a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js +++ b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js @@ -302,7 +302,7 @@ describe('useMutableSource', () => { />, () => Scheduler.unstable_yieldValue('Sync effect'), ); - expect(Scheduler).toFlushAndYieldThrough(['only:a-one', 'Sync effect']); + expect(Scheduler).toFlushAndYield(['only:a-one', 'Sync effect']); ReactNoop.flushPassiveEffects(); expect(sourceA.listenerCount).toBe(1);