Repository: facebook/react
Pull request: Always keep disabled logs in the second pass

For some class methods, we were disabling logs during the first rather than the second render. This can cover up issues if we deduplicate warnings. To fix, swap the order so the first render is consistently the one that has the logs.

Changed files:
filename: packages/react-reconciler/src/ReactFiberClassComponent.new.js
patch: @@ -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

filename: packages/react-reconciler/src/ReactFiberClassComponent.old.js
patch: @@ -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

filename: packages/react/src/__tests__/ReactStrictMode-test.js
patch: @@ -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');
+    });
+  });
 });