Fixed high pri updates erasing pending lower pri subscription updates

facebook · Feb 10, 2020 · bf33e75 · bf33e75
1 parent 1ae01f6
commit bf33e75
Show file tree

Hide file tree

Showing 2 changed files with 13 additions and 4 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js
@@ -900,6 +900,12 @@ function useMutableSourceImpl<S>(
     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<S>);

diff --git 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', () => {