Use SameValue instead of === to check for dispatchAction equivalence (#…

…14752)
facebook · Feb 4, 2019 · e602b52 · e602b52
1 parent e489c3f
commit e602b52
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 3 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js
@@ -612,7 +612,7 @@ function updateReducer<S, I, A>(
 
         // Mark that the fiber performed work, but only if the new state is
         // different from the current state.
-        if (newState !== hook.memoizedState) {
+        if (!is(newState, hook.memoizedState)) {
           markWorkInProgressReceivedUpdate();
         }
 
@@ -695,7 +695,7 @@ function updateReducer<S, I, A>(
 
     // Mark that the fiber performed work, but only if the new state is
     // different from the current state.
-    if (newState !== hook.memoizedState) {
+    if (!is(newState, hook.memoizedState)) {
       markWorkInProgressReceivedUpdate();
     }
 
@@ -1105,7 +1105,7 @@ function dispatchAction<S, A>(
           // without calling the reducer again.
           update.eagerReducer = eagerReducer;
           update.eagerState = eagerState;
-          if (eagerState === currentState) {
+          if (is(eagerState, currentState)) {
             // Fast path. We can bail out without scheduling React to re-render.
             // It's still possible that we'll need to rebase this update later,
             // if the component re-renders for a different reason and by that

diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
@@ -114,6 +114,30 @@ describe('ReactHooks', () => {
     // Because the final values are the same as the current values, the
     // component bails out.
     expect(root).toFlushAndYield(['Parent: 1, 2']);
+
+    // prepare to check SameValue
+    setCounter1(0 / -1);
+    setCounter2(NaN);
+    expect(root).toFlushAndYield([
+      'Parent: 0, NaN',
+      'Child: 0, NaN',
+      'Effect: 0, NaN',
+    ]);
+
+    // check if re-setting to negative 0 / NaN still bails out
+    setCounter1(0 / -1);
+    setCounter2(NaN);
+    setCounter2(Infinity);
+    setCounter2(NaN);
+    expect(root).toFlushAndYield(['Parent: 0, NaN']);
+
+    // check if changing negative 0 to positive 0 does not bail out
+    setCounter1(0);
+    expect(root).toFlushAndYield([
+      'Parent: 0, NaN',
+      'Child: 0, NaN',
+      'Effect: 0, NaN',
+    ]);
   });
 
   it('bails out in render phase if all the state is the same and props bail out with memo', () => {
@@ -375,6 +399,26 @@ describe('ReactHooks', () => {
     setCounter(2);
     expect(root).toFlushAndYield(['Parent: 2', 'Child: 2', 'Effect: 2']);
     expect(root).toMatchRenderedOutput('2');
+
+    // prepare to check SameValue
+    setCounter(0);
+    expect(root).toFlushAndYield(['Parent: 0', 'Child: 0', 'Effect: 0']);
+    expect(root).toMatchRenderedOutput('0');
+
+    // Update to the same state for the first time to flush the queue
+    setCounter(0);
+    expect(root).toFlushAndYield(['Parent: 0']);
+    expect(root).toMatchRenderedOutput('0');
+
+    // Update again to the same state. Should bail out.
+    setCounter(0);
+    expect(root).toFlushAndYield([]);
+    expect(root).toMatchRenderedOutput('0');
+
+    // Update to a different state (positive 0 to negative 0)
+    setCounter(0 / -1);
+    expect(root).toFlushAndYield(['Parent: 0', 'Child: 0', 'Effect: 0']);
+    expect(root).toMatchRenderedOutput('0');
   });
 
   it('bails out multiple times in a row without entering render phase', () => {