Always keep disabled logs in the second pass (#21739)

* Add tests for disabled logs * Always keep disabled logs in the second pass * Jest nit * Always use the second result
facebook · Jun 24, 2021 · 51b0bec · 51b0bec
1 parent 386e8f2
commit 51b0bec
Show file tree

Hide file tree

Showing 3 changed files with 220 additions and 36 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.new.js b/packages/react-reconciler/src/ReactFiberClassComponent.new.js
@@ -169,7 +169,7 @@ function applyDerivedStateFromProps(
   nextProps: any,
 ) {
   const prevState = workInProgress.memoizedState;
-
+  let partialState = getDerivedStateFromProps(nextProps, prevState);
   if (__DEV__) {
     if (
       debugRenderPhaseSideEffectsForStrictMode &&
@@ -178,16 +178,11 @@ function applyDerivedStateFromProps(
       disableLogs();
       try {
         // Invoke the function an extra time to help detect side-effects.
-        getDerivedStateFromProps(nextProps, prevState);
+        partialState = getDerivedStateFromProps(nextProps, prevState);
       } finally {
         reenableLogs();
       }
     }
-  }
-
-  const partialState = getDerivedStateFromProps(nextProps, prevState);
-
-  if (__DEV__) {
     warnOnUndefinedDerivedState(ctor, partialState);
   }
   // Merge the partial state and the previous state.
@@ -323,6 +318,11 @@ function checkShouldComponentUpdate(
 ) {
   const instance = workInProgress.stateNode;
   if (typeof instance.shouldComponentUpdate === 'function') {
+    let shouldUpdate = instance.shouldComponentUpdate(
+      newProps,
+      newState,
+      nextContext,
+    );
     if (__DEV__) {
       if (
         debugRenderPhaseSideEffectsForStrictMode &&
@@ -331,19 +331,15 @@ function checkShouldComponentUpdate(
         disableLogs();
         try {
           // Invoke the function an extra time to help detect side-effects.
-          instance.shouldComponentUpdate(newProps, newState, nextContext);
+          shouldUpdate = instance.shouldComponentUpdate(
+            newProps,
+            newState,
+            nextContext,
+          );
         } finally {
           reenableLogs();
         }
       }
-    }
-    const shouldUpdate = instance.shouldComponentUpdate(
-      newProps,
-      newState,
-      nextContext,
-    );
-
-    if (__DEV__) {
       if (shouldUpdate === undefined) {
         console.error(
           '%s.shouldComponentUpdate(): Returned undefined instead of a ' +
@@ -659,6 +655,7 @@ function constructClassInstance(
       : emptyContextObject;
   }
 
+  let instance = new ctor(props, context);
   // Instantiate twice to help detect side-effects.
   if (__DEV__) {
     if (
@@ -667,14 +664,13 @@ function constructClassInstance(
     ) {
       disableLogs();
       try {
-        new ctor(props, context); // eslint-disable-line no-new
+        instance = new ctor(props, context); // eslint-disable-line no-new
       } finally {
         reenableLogs();
       }
     }
   }
 
-  const instance = new ctor(props, context);
   const state = (workInProgress.memoizedState =
     instance.state !== null && instance.state !== undefined
       ? instance.state

diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.old.js b/packages/react-reconciler/src/ReactFiberClassComponent.old.js
@@ -169,7 +169,7 @@ function applyDerivedStateFromProps(
   nextProps: any,
 ) {
   const prevState = workInProgress.memoizedState;
-
+  let partialState = getDerivedStateFromProps(nextProps, prevState);
   if (__DEV__) {
     if (
       debugRenderPhaseSideEffectsForStrictMode &&
@@ -178,16 +178,11 @@ function applyDerivedStateFromProps(
       disableLogs();
       try {
         // Invoke the function an extra time to help detect side-effects.
-        getDerivedStateFromProps(nextProps, prevState);
+        partialState = getDerivedStateFromProps(nextProps, prevState);
       } finally {
         reenableLogs();
       }
     }
-  }
-
-  const partialState = getDerivedStateFromProps(nextProps, prevState);
-
-  if (__DEV__) {
     warnOnUndefinedDerivedState(ctor, partialState);
   }
   // Merge the partial state and the previous state.
@@ -323,6 +318,11 @@ function checkShouldComponentUpdate(
 ) {
   const instance = workInProgress.stateNode;
   if (typeof instance.shouldComponentUpdate === 'function') {
+    let shouldUpdate = instance.shouldComponentUpdate(
+      newProps,
+      newState,
+      nextContext,
+    );
     if (__DEV__) {
       if (
         debugRenderPhaseSideEffectsForStrictMode &&
@@ -331,19 +331,15 @@ function checkShouldComponentUpdate(
         disableLogs();
         try {
           // Invoke the function an extra time to help detect side-effects.
-          instance.shouldComponentUpdate(newProps, newState, nextContext);
+          shouldUpdate = instance.shouldComponentUpdate(
+            newProps,
+            newState,
+            nextContext,
+          );
         } finally {
           reenableLogs();
         }
       }
-    }
-    const shouldUpdate = instance.shouldComponentUpdate(
-      newProps,
-      newState,
-      nextContext,
-    );
-
-    if (__DEV__) {
       if (shouldUpdate === undefined) {
         console.error(
           '%s.shouldComponentUpdate(): Returned undefined instead of a ' +
@@ -659,6 +655,7 @@ function constructClassInstance(
       : emptyContextObject;
   }
 
+  let instance = new ctor(props, context);
   // Instantiate twice to help detect side-effects.
   if (__DEV__) {
     if (
@@ -667,14 +664,13 @@ function constructClassInstance(
     ) {
       disableLogs();
       try {
-        new ctor(props, context); // eslint-disable-line no-new
+        instance = new ctor(props, context); // eslint-disable-line no-new
       } finally {
         reenableLogs();
       }
     }
   }
 
-  const instance = new ctor(props, context);
   const state = (workInProgress.memoizedState =
     instance.state !== null && instance.state !== undefined
       ? instance.state

diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js
@@ -892,4 +892,196 @@ describe('context legacy', () => {
     // Dedupe
     ReactDOM.render(<Root />, container);
   });
+
+  describe('disableLogs', () => {
+    it('disables logs once for class double render', () => {
+      spyOnDevAndProd(console, 'log');
+
+      let count = 0;
+      class Foo extends React.Component {
+        render() {
+          count++;
+          console.log('foo ' + count);
+          return null;
+        }
+      }
+
+      const container = document.createElement('div');
+      ReactDOM.render(
+        <React.StrictMode>
+          <Foo />
+        </React.StrictMode>,
+        container,
+      );
+
+      expect(count).toBe(__DEV__ ? 2 : 1);
+      expect(console.log).toBeCalledTimes(1);
+      // Note: we should display the first log because otherwise
+      // there is a risk of suppressing warnings when they happen,
+      // and on the next render they'd get deduplicated and ignored.
+      expect(console.log).toBeCalledWith('foo 1');
+    });
+
+    it('disables logs once for class double ctor', () => {
+      spyOnDevAndProd(console, 'log');
+
+      let count = 0;
+      class Foo extends React.Component {
+        constructor(props) {
+          super(props);
+          count++;
+          console.log('foo ' + count);
+        }
+        render() {
+          return null;
+        }
+      }
+
+      const container = document.createElement('div');
+      ReactDOM.render(
+        <React.StrictMode>
+          <Foo />
+        </React.StrictMode>,
+        container,
+      );
+
+      expect(count).toBe(__DEV__ ? 2 : 1);
+      expect(console.log).toBeCalledTimes(1);
+      // Note: we should display the first log because otherwise
+      // there is a risk of suppressing warnings when they happen,
+      // and on the next render they'd get deduplicated and ignored.
+      expect(console.log).toBeCalledWith('foo 1');
+    });
+
+    it('disables logs once for class double getDerivedStateFromProps', () => {
+      spyOnDevAndProd(console, 'log');
+
+      let count = 0;
+      class Foo extends React.Component {
+        state = {};
+        static getDerivedStateFromProps() {
+          count++;
+          console.log('foo ' + count);
+          return {};
+        }
+        render() {
+          return null;
+        }
+      }
+
+      const container = document.createElement('div');
+      ReactDOM.render(
+        <React.StrictMode>
+          <Foo />
+        </React.StrictMode>,
+        container,
+      );
+
+      expect(count).toBe(__DEV__ ? 2 : 1);
+      expect(console.log).toBeCalledTimes(1);
+      // Note: we should display the first log because otherwise
+      // there is a risk of suppressing warnings when they happen,
+      // and on the next render they'd get deduplicated and ignored.
+      expect(console.log).toBeCalledWith('foo 1');
+    });
+
+    it('disables logs once for class double shouldComponentUpdate', () => {
+      spyOnDevAndProd(console, 'log');
+
+      let count = 0;
+      class Foo extends React.Component {
+        state = {};
+        shouldComponentUpdate() {
+          count++;
+          console.log('foo ' + count);
+          return {};
+        }
+        render() {
+          return null;
+        }
+      }
+
+      const container = document.createElement('div');
+      ReactDOM.render(
+        <React.StrictMode>
+          <Foo />
+        </React.StrictMode>,
+        container,
+      );
+      // Trigger sCU:
+      ReactDOM.render(
+        <React.StrictMode>
+          <Foo />
+        </React.StrictMode>,
+        container,
+      );
+
+      expect(count).toBe(__DEV__ ? 2 : 1);
+      expect(console.log).toBeCalledTimes(1);
+      // Note: we should display the first log because otherwise
+      // there is a risk of suppressing warnings when they happen,
+      // and on the next render they'd get deduplicated and ignored.
+      expect(console.log).toBeCalledWith('foo 1');
+    });
+
+    it('disables logs once for class state updaters', () => {
+      spyOnDevAndProd(console, 'log');
+
+      let inst;
+      let count = 0;
+      class Foo extends React.Component {
+        state = {};
+        render() {
+          inst = this;
+          return null;
+        }
+      }
+
+      const container = document.createElement('div');
+      ReactDOM.render(
+        <React.StrictMode>
+          <Foo />
+        </React.StrictMode>,
+        container,
+      );
+      inst.setState(() => {
+        count++;
+        console.log('foo ' + count);
+        return {};
+      });
+
+      expect(count).toBe(__DEV__ ? 2 : 1);
+      expect(console.log).toBeCalledTimes(1);
+      // Note: we should display the first log because otherwise
+      // there is a risk of suppressing warnings when they happen,
+      // and on the next render they'd get deduplicated and ignored.
+      expect(console.log).toBeCalledWith('foo 1');
+    });
+
+    it('disables logs once for function double render', () => {
+      spyOnDevAndProd(console, 'log');
+
+      let count = 0;
+      function Foo() {
+        count++;
+        console.log('foo ' + count);
+        return null;
+      }
+
+      const container = document.createElement('div');
+      ReactDOM.render(
+        <React.StrictMode>
+          <Foo />
+        </React.StrictMode>,
+        container,
+      );
+
+      expect(count).toBe(__DEV__ ? 2 : 1);
+      expect(console.log).toBeCalledTimes(1);
+      // Note: we should display the first log because otherwise
+      // there is a risk of suppressing warnings when they happen,
+      // and on the next render they'd get deduplicated and ignored.
+      expect(console.log).toBeCalledWith('foo 1');
+    });
+  });
 });