From 0992d099cee25d94846295461192136a94562f24 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 23 Jun 2021 21:18:43 +0100 Subject: [PATCH 1/4] Add tests for disabled logs --- .../src/__tests__/ReactStrictMode-test.js | 186 ++++++++++++++++++ 1 file changed, 186 insertions(+) diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index a41f2ef2ccc75..886a552536f0a 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -892,4 +892,190 @@ describe('context legacy', () => { // Dedupe ReactDOM.render(, 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( + + + , + container, + ); + + expect(count).toBe(__DEV__ ? 2 : 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.calls.all().map(c => c.args)).toEqual([['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( + + + , + container, + ); + + expect(count).toBe(__DEV__ ? 2 : 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.calls.all().map(c => c.args)).toEqual([['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( + + + , + container, + ); + + expect(count).toBe(__DEV__ ? 2 : 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.calls.all().map(c => c.args)).toEqual([['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( + + + , + container, + ); + // Trigger sCU: + ReactDOM.render( + + + , + container, + ); + + expect(count).toBe(__DEV__ ? 2 : 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.calls.all().map(c => c.args)).toEqual([['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( + + + , + container, + ); + inst.setState(() => { + count++; + console.log('foo ' + count); + return {}; + }); + + expect(count).toBe(__DEV__ ? 2 : 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.calls.all().map(c => c.args)).toEqual([['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( + + + , + container, + ); + + expect(count).toBe(__DEV__ ? 2 : 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.calls.all().map(c => c.args)).toEqual([['foo 1']]); + }); + }); }); From aec503f2ee6e933123482a1e751b73a08d5f3e13 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 23 Jun 2021 21:38:09 +0100 Subject: [PATCH 2/4] Always keep disabled logs in the second pass --- .../src/ReactFiberClassComponent.new.js | 38 ++++++++----------- .../src/ReactFiberClassComponent.old.js | 38 ++++++++----------- 2 files changed, 30 insertions(+), 46 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.new.js b/packages/react-reconciler/src/ReactFiberClassComponent.new.js index c464b6c3bf411..bb76f70072d46 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.new.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.new.js @@ -169,8 +169,9 @@ function applyDerivedStateFromProps( nextProps: any, ) { const prevState = workInProgress.memoizedState; - + const partialState = getDerivedStateFromProps(nextProps, prevState); if (__DEV__) { + warnOnUndefinedDerivedState(ctor, partialState); if ( debugRenderPhaseSideEffectsForStrictMode && workInProgress.mode & StrictLegacyMode @@ -184,12 +185,6 @@ function applyDerivedStateFromProps( } } } - - const partialState = getDerivedStateFromProps(nextProps, prevState); - - if (__DEV__) { - warnOnUndefinedDerivedState(ctor, partialState); - } // Merge the partial state and the previous state. const memoizedState = partialState === null || partialState === undefined @@ -323,26 +318,11 @@ function checkShouldComponentUpdate( ) { const instance = workInProgress.stateNode; if (typeof instance.shouldComponentUpdate === 'function') { - if (__DEV__) { - if ( - debugRenderPhaseSideEffectsForStrictMode && - workInProgress.mode & StrictLegacyMode - ) { - disableLogs(); - try { - // Invoke the function an extra time to help detect side-effects. - instance.shouldComponentUpdate(newProps, newState, nextContext); - } finally { - reenableLogs(); - } - } - } const shouldUpdate = instance.shouldComponentUpdate( newProps, newState, nextContext, ); - if (__DEV__) { if (shouldUpdate === undefined) { console.error( @@ -351,6 +331,18 @@ function checkShouldComponentUpdate( getComponentNameFromType(ctor) || 'Component', ); } + if ( + debugRenderPhaseSideEffectsForStrictMode && + workInProgress.mode & StrictLegacyMode + ) { + disableLogs(); + try { + // Invoke the function an extra time to help detect side-effects. + instance.shouldComponentUpdate(newProps, newState, nextContext); + } finally { + reenableLogs(); + } + } } return shouldUpdate; @@ -659,6 +651,7 @@ function constructClassInstance( : emptyContextObject; } + const instance = new ctor(props, context); // Instantiate twice to help detect side-effects. if (__DEV__) { if ( @@ -674,7 +667,6 @@ function constructClassInstance( } } - 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 index adb616c6262b9..4a62449004b90 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.old.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.old.js @@ -169,8 +169,9 @@ function applyDerivedStateFromProps( nextProps: any, ) { const prevState = workInProgress.memoizedState; - + const partialState = getDerivedStateFromProps(nextProps, prevState); if (__DEV__) { + warnOnUndefinedDerivedState(ctor, partialState); if ( debugRenderPhaseSideEffectsForStrictMode && workInProgress.mode & StrictLegacyMode @@ -184,12 +185,6 @@ function applyDerivedStateFromProps( } } } - - const partialState = getDerivedStateFromProps(nextProps, prevState); - - if (__DEV__) { - warnOnUndefinedDerivedState(ctor, partialState); - } // Merge the partial state and the previous state. const memoizedState = partialState === null || partialState === undefined @@ -323,26 +318,11 @@ function checkShouldComponentUpdate( ) { const instance = workInProgress.stateNode; if (typeof instance.shouldComponentUpdate === 'function') { - if (__DEV__) { - if ( - debugRenderPhaseSideEffectsForStrictMode && - workInProgress.mode & StrictLegacyMode - ) { - disableLogs(); - try { - // Invoke the function an extra time to help detect side-effects. - instance.shouldComponentUpdate(newProps, newState, nextContext); - } finally { - reenableLogs(); - } - } - } const shouldUpdate = instance.shouldComponentUpdate( newProps, newState, nextContext, ); - if (__DEV__) { if (shouldUpdate === undefined) { console.error( @@ -351,6 +331,18 @@ function checkShouldComponentUpdate( getComponentNameFromType(ctor) || 'Component', ); } + if ( + debugRenderPhaseSideEffectsForStrictMode && + workInProgress.mode & StrictLegacyMode + ) { + disableLogs(); + try { + // Invoke the function an extra time to help detect side-effects. + instance.shouldComponentUpdate(newProps, newState, nextContext); + } finally { + reenableLogs(); + } + } } return shouldUpdate; @@ -659,6 +651,7 @@ function constructClassInstance( : emptyContextObject; } + const instance = new ctor(props, context); // Instantiate twice to help detect side-effects. if (__DEV__) { if ( @@ -674,7 +667,6 @@ function constructClassInstance( } } - const instance = new ctor(props, context); const state = (workInProgress.memoizedState = instance.state !== null && instance.state !== undefined ? instance.state From 96d2a7bda043b7c0eb06f41c60025d2fef34fb41 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 24 Jun 2021 15:32:25 +0100 Subject: [PATCH 3/4] Jest nit --- .../src/__tests__/ReactStrictMode-test.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index 886a552536f0a..5bd16f9c544dd 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -915,10 +915,11 @@ describe('context legacy', () => { ); 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.calls.all().map(c => c.args)).toEqual([['foo 1']]); + expect(console.log).toBeCalledWith('foo 1'); }); it('disables logs once for class double ctor', () => { @@ -945,10 +946,11 @@ describe('context legacy', () => { ); 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.calls.all().map(c => c.args)).toEqual([['foo 1']]); + expect(console.log).toBeCalledWith('foo 1'); }); it('disables logs once for class double getDerivedStateFromProps', () => { @@ -976,10 +978,11 @@ describe('context legacy', () => { ); 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.calls.all().map(c => c.args)).toEqual([['foo 1']]); + expect(console.log).toBeCalledWith('foo 1'); }); it('disables logs once for class double shouldComponentUpdate', () => { @@ -1014,10 +1017,11 @@ describe('context legacy', () => { ); 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.calls.all().map(c => c.args)).toEqual([['foo 1']]); + expect(console.log).toBeCalledWith('foo 1'); }); it('disables logs once for class state updaters', () => { @@ -1047,10 +1051,11 @@ describe('context legacy', () => { }); 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.calls.all().map(c => c.args)).toEqual([['foo 1']]); + expect(console.log).toBeCalledWith('foo 1'); }); it('disables logs once for function double render', () => { @@ -1072,10 +1077,11 @@ describe('context legacy', () => { ); 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.calls.all().map(c => c.args)).toEqual([['foo 1']]); + expect(console.log).toBeCalledWith('foo 1'); }); }); }); From adfa3d47ac37af27c8ae58e06e7ce32279cbf64b Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 24 Jun 2021 19:12:42 +0100 Subject: [PATCH 4/4] Always use the second result --- .../src/ReactFiberClassComponent.new.js | 32 +++++++++++-------- .../src/ReactFiberClassComponent.old.js | 32 +++++++++++-------- 2 files changed, 36 insertions(+), 28 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.new.js b/packages/react-reconciler/src/ReactFiberClassComponent.new.js index bb76f70072d46..3d10472c0441c 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.new.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.new.js @@ -169,9 +169,8 @@ function applyDerivedStateFromProps( nextProps: any, ) { const prevState = workInProgress.memoizedState; - const partialState = getDerivedStateFromProps(nextProps, prevState); + let partialState = getDerivedStateFromProps(nextProps, prevState); if (__DEV__) { - warnOnUndefinedDerivedState(ctor, partialState); if ( debugRenderPhaseSideEffectsForStrictMode && workInProgress.mode & StrictLegacyMode @@ -179,11 +178,12 @@ function applyDerivedStateFromProps( disableLogs(); try { // Invoke the function an extra time to help detect side-effects. - getDerivedStateFromProps(nextProps, prevState); + partialState = getDerivedStateFromProps(nextProps, prevState); } finally { reenableLogs(); } } + warnOnUndefinedDerivedState(ctor, partialState); } // Merge the partial state and the previous state. const memoizedState = @@ -318,19 +318,12 @@ function checkShouldComponentUpdate( ) { const instance = workInProgress.stateNode; if (typeof instance.shouldComponentUpdate === 'function') { - const shouldUpdate = instance.shouldComponentUpdate( + let shouldUpdate = instance.shouldComponentUpdate( newProps, newState, nextContext, ); if (__DEV__) { - if (shouldUpdate === undefined) { - console.error( - '%s.shouldComponentUpdate(): Returned undefined instead of a ' + - 'boolean value. Make sure to return true or false.', - getComponentNameFromType(ctor) || 'Component', - ); - } if ( debugRenderPhaseSideEffectsForStrictMode && workInProgress.mode & StrictLegacyMode @@ -338,11 +331,22 @@ 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(); } } + if (shouldUpdate === undefined) { + console.error( + '%s.shouldComponentUpdate(): Returned undefined instead of a ' + + 'boolean value. Make sure to return true or false.', + getComponentNameFromType(ctor) || 'Component', + ); + } } return shouldUpdate; @@ -651,7 +655,7 @@ function constructClassInstance( : emptyContextObject; } - const instance = new ctor(props, context); + let instance = new ctor(props, context); // Instantiate twice to help detect side-effects. if (__DEV__) { if ( @@ -660,7 +664,7 @@ 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(); } diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.old.js b/packages/react-reconciler/src/ReactFiberClassComponent.old.js index 4a62449004b90..ab80913c7b6b6 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.old.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.old.js @@ -169,9 +169,8 @@ function applyDerivedStateFromProps( nextProps: any, ) { const prevState = workInProgress.memoizedState; - const partialState = getDerivedStateFromProps(nextProps, prevState); + let partialState = getDerivedStateFromProps(nextProps, prevState); if (__DEV__) { - warnOnUndefinedDerivedState(ctor, partialState); if ( debugRenderPhaseSideEffectsForStrictMode && workInProgress.mode & StrictLegacyMode @@ -179,11 +178,12 @@ function applyDerivedStateFromProps( disableLogs(); try { // Invoke the function an extra time to help detect side-effects. - getDerivedStateFromProps(nextProps, prevState); + partialState = getDerivedStateFromProps(nextProps, prevState); } finally { reenableLogs(); } } + warnOnUndefinedDerivedState(ctor, partialState); } // Merge the partial state and the previous state. const memoizedState = @@ -318,19 +318,12 @@ function checkShouldComponentUpdate( ) { const instance = workInProgress.stateNode; if (typeof instance.shouldComponentUpdate === 'function') { - const shouldUpdate = instance.shouldComponentUpdate( + let shouldUpdate = instance.shouldComponentUpdate( newProps, newState, nextContext, ); if (__DEV__) { - if (shouldUpdate === undefined) { - console.error( - '%s.shouldComponentUpdate(): Returned undefined instead of a ' + - 'boolean value. Make sure to return true or false.', - getComponentNameFromType(ctor) || 'Component', - ); - } if ( debugRenderPhaseSideEffectsForStrictMode && workInProgress.mode & StrictLegacyMode @@ -338,11 +331,22 @@ 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(); } } + if (shouldUpdate === undefined) { + console.error( + '%s.shouldComponentUpdate(): Returned undefined instead of a ' + + 'boolean value. Make sure to return true or false.', + getComponentNameFromType(ctor) || 'Component', + ); + } } return shouldUpdate; @@ -651,7 +655,7 @@ function constructClassInstance( : emptyContextObject; } - const instance = new ctor(props, context); + let instance = new ctor(props, context); // Instantiate twice to help detect side-effects. if (__DEV__) { if ( @@ -660,7 +664,7 @@ 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(); }