From 57c32b148fd7f140393adb8ca7fba28ce40e0647 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 10 Feb 2020 14:21:55 -0800 Subject: [PATCH] Fixed high pri updates erasing pending lower pri subscription updates --- packages/react-reconciler/src/ReactFiberHooks.js | 6 ++++++ .../ReactHooksWithNoopRenderer-test.internal.js | 11 +++++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index b3c4443b8590f..21d3fa3518333 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -900,6 +900,12 @@ function useMutableSourceImpl( isSafeToReadFromSource = pendingExpirationTime === NoWork || pendingExpirationTime >= expirationTime; + + if (!isSafeToReadFromSource) { + // Preserve the pending update if the current priority doesn't include it. + currentlyRenderingFiber.expirationTime = pendingExpirationTime; + markUnprocessedUpdateTime(pendingExpirationTime); + } } let prevMemoizedState = ((hook.memoizedState: any): ?MutableSourceState); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index b887f20b736ad..52f5acaba6676 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -2016,7 +2016,6 @@ describe('ReactHooksWithNoopRenderer', () => { () => Scheduler.unstable_yieldValue('Sync effect'), ); expect(Scheduler).toFlushAndYield(['a:one', 'b:one', 'Sync effect']); - ReactNoop.flushPassiveEffects(); // Changing values should schedule an update with React. // Start working on this update but don't finish it. @@ -2036,9 +2035,7 @@ describe('ReactHooksWithNoopRenderer', () => { }); expect(Scheduler).toHaveYielded(['a:one', 'b:one', 'Sync effect']); - // TODO (useMutableSource) Re-enable the assertion below; it fails now for reasons unknown. - // Once the update is processed, the new value should be used - // expect(Scheduler).toFlushAndYield(['a:two', 'b:two']); + expect(Scheduler).toFlushAndYield(['a:two', 'b:two']); }); it('should read from source on newly mounted subtree if no pending updates are scheduled for source', () => { @@ -2098,6 +2095,12 @@ describe('ReactHooksWithNoopRenderer', () => { }); expect(Scheduler).toHaveYielded(['a:two', 'b:two', 'Sync effect']); }); + + // TODO (useMutableSource) Test case for a scoped subscription, + // followed by a render that reads from a different part of the store, + // with a mutation between its first and second read, + // that isn't picked up on by the scoped subscription, + // to verify that we also use the version number to protect against this case. }); describe('useCallback', () => {