From f5bb22f9553dd3bb4417d3f3419a47f59b9b330b Mon Sep 17 00:00:00 2001 From: Anushree Subramani Date: Wed, 11 Oct 2017 18:09:12 +0530 Subject: [PATCH 1/9] Deduplicated many warnings (#11140) * Deduplicated the following warnings: 1. Can only update a mounted or mounting component. This usually means you called setState, replaceState, or forceUpdate on an unmounted component. This is a no-op 2. %s.componentWillReceiveProps(): Assigning directly to this.state is deprecated (except inside a component's constructor). Use setState instead.' 3. An update (setState, replaceState, or forceUpdate) was scheduled from inside an update function. Update functions should be pure, with zero side-effects. Consider using componentDidUpdate or a callback. 4. setState(...): Cannot call setState() inside getChildContext() * Code review changes made for #11140 --- .../src/server/ReactPartialRenderer.js | 11 ++++++- .../src/ReactFiberClassComponent.js | 19 +++++++----- .../src/ReactFiberScheduler.js | 31 ++++++++++++------- .../src/ReactFiberUpdateQueue.js | 7 ++++- packages/react/src/ReactNoopUpdateQueue.js | 12 +++++-- 5 files changed, 58 insertions(+), 22 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 62ea2e04282..254d049cc42 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -72,6 +72,7 @@ if (__DEV__) { var {ReactDebugCurrentFrame} = require('shared/ReactGlobalSharedState'); var currentDebugStack = null; var currentDebugElementStack = null; + var didWarnAboutNoopUpdateForComponent = {}; var setCurrentDebugStack = function(stack: Array) { var frame: Frame = stack[stack.length - 1]; currentDebugElementStack = ((frame: any): FrameDev).debugElementStack; @@ -175,6 +176,13 @@ function warnNoop( ) { if (__DEV__) { var constructor = publicInstance.constructor; + const componentName = + (constructor && getComponentName(constructor)) || 'ReactClass'; + const warningKey = `${callerName}_${componentName}`; + if (didWarnAboutNoopUpdateForComponent[warningKey]) { + return; + } + warning( false, '%s(...): Can only update a mounting component. ' + @@ -182,8 +190,9 @@ function warnNoop( 'This is a no-op.\n\nPlease check the code for the %s component.', callerName, callerName, - (constructor && getComponentName(constructor)) || 'ReactClass', + componentName, ); + didWarnAboutNoopUpdateForComponent[warningKey] = true; } } diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 396be01fb8a..2a1e9a64ee3 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -41,6 +41,7 @@ if (__DEV__) { var warning = require('fbjs/lib/warning'); var {startPhaseTimer, stopPhaseTimer} = require('./ReactDebugFiberPerf'); + var didWarnAboutStateAssignmentForComponent = {}; var warnOnInvalidCallback = function(callback: mixed, callerName: string) { warning( @@ -393,13 +394,17 @@ module.exports = function( if (instance.state !== oldState) { if (__DEV__) { - warning( - false, - '%s.componentWillReceiveProps(): Assigning directly to ' + - "this.state is deprecated (except inside a component's " + - 'constructor). Use setState instead.', - getComponentName(workInProgress), - ); + const componentName = getComponentName(workInProgress) || 'Component'; + if (!didWarnAboutStateAssignmentForComponent[componentName]) { + warning( + false, + '%s.componentWillReceiveProps(): Assigning directly to ' + + "this.state is deprecated (except inside a component's " + + 'constructor). Use setState instead.', + componentName, + ); + didWarnAboutStateAssignmentForComponent[componentName] = true; + } } updater.enqueueReplaceState(instance, instance.state, null); } diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 67a87c9a4a3..21f92db452a 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -101,34 +101,42 @@ if (__DEV__) { } = require('./ReactDebugFiberPerf'); var didWarnAboutStateTransition = false; + var didWarnSetStateChildContext = false; + var didWarnStateUpdateForUnmountedComponent = {}; + + var warnAboutUpdateOnUnmounted = function(fiber: Fiber) { + const componentName = getComponentName(fiber) || 'ReactClass'; + if (didWarnStateUpdateForUnmountedComponent[componentName]) { + return; + } - var warnAboutUpdateOnUnmounted = function( - instance: React$ComponentType, - ) { - const ctor = instance.constructor; warning( false, - 'Can only update a mounted or mounting component. This usually means ' + - 'you called setState, replaceState, or forceUpdate on an unmounted ' + - 'component. This is a no-op.\n\nPlease check the code for the ' + - '%s component.', - (ctor && (ctor.displayName || ctor.name)) || 'ReactClass', + 'Can only update a mounted or mounting ' + + 'component. This usually means you called setState, replaceState, ' + + 'or forceUpdate on an unmounted component. This is a no-op.\n\nPlease ' + + 'check the code for the %s component.', + componentName, ); + didWarnStateUpdateForUnmountedComponent[componentName] = true; }; var warnAboutInvalidUpdates = function(instance: React$ComponentType) { switch (ReactDebugCurrentFiber.phase) { case 'getChildContext': + if (didWarnSetStateChildContext) { + return; + } warning( false, 'setState(...): Cannot call setState() inside getChildContext()', ); + didWarnSetStateChildContext = true; break; case 'render': if (didWarnAboutStateTransition) { return; } - didWarnAboutStateTransition = true; warning( false, 'Cannot update during an existing state transition (such as within ' + @@ -136,6 +144,7 @@ if (__DEV__) { 'be a pure function of props and state; constructor side-effects are ' + 'an anti-pattern, but can be moved to `componentWillMount`.', ); + didWarnAboutStateTransition = true; break; } }; @@ -1229,7 +1238,7 @@ module.exports = function( } else { if (__DEV__) { if (!isErrorRecovery && fiber.tag === ClassComponent) { - warnAboutUpdateOnUnmounted(fiber.stateNode); + warnAboutUpdateOnUnmounted(fiber); } } return; diff --git a/packages/react-reconciler/src/ReactFiberUpdateQueue.js b/packages/react-reconciler/src/ReactFiberUpdateQueue.js index 1fd6fa18cb8..9773bdc9e83 100644 --- a/packages/react-reconciler/src/ReactFiberUpdateQueue.js +++ b/packages/react-reconciler/src/ReactFiberUpdateQueue.js @@ -20,6 +20,7 @@ const {NoWork} = require('./ReactFiberExpirationTime'); if (__DEV__) { var warning = require('fbjs/lib/warning'); + var didWarnUpdateInsideUpdate = false; } type PartialState = @@ -132,7 +133,10 @@ function insertUpdateIntoFiber( // Warn if an update is scheduled from inside an updater function. if (__DEV__) { - if (queue1.isProcessing || (queue2 !== null && queue2.isProcessing)) { + if ( + (queue1.isProcessing || (queue2 !== null && queue2.isProcessing)) && + !didWarnUpdateInsideUpdate + ) { warning( false, 'An update (setState, replaceState, or forceUpdate) was scheduled ' + @@ -140,6 +144,7 @@ function insertUpdateIntoFiber( 'with zero side-effects. Consider using componentDidUpdate or a ' + 'callback.', ); + didWarnUpdateInsideUpdate = true; } } diff --git a/packages/react/src/ReactNoopUpdateQueue.js b/packages/react/src/ReactNoopUpdateQueue.js index 5b0631dd086..56391be1abb 100644 --- a/packages/react/src/ReactNoopUpdateQueue.js +++ b/packages/react/src/ReactNoopUpdateQueue.js @@ -9,11 +9,19 @@ if (__DEV__) { var warning = require('fbjs/lib/warning'); + var didWarnStateUpdateForUnmountedComponent = {}; } function warnNoop(publicInstance, callerName) { if (__DEV__) { var constructor = publicInstance.constructor; + const componentName = + (constructor && (constructor.displayName || constructor.name)) || + 'ReactClass'; + const warningKey = `${callerName}_${componentName}`; + if (didWarnStateUpdateForUnmountedComponent[warningKey]) { + return; + } warning( false, '%s(...): Can only update a mounted or mounting component. ' + @@ -21,9 +29,9 @@ function warnNoop(publicInstance, callerName) { 'This is a no-op.\n\nPlease check the code for the %s component.', callerName, callerName, - (constructor && (constructor.displayName || constructor.name)) || - 'ReactClass', + componentName, ); + didWarnStateUpdateForUnmountedComponent[warningKey] = true; } } From 415a864dfd850e877c9f82caa92e520c2d72eafd Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 12:06:53 +0000 Subject: [PATCH 2/9] Minor style fix --- packages/react-dom/src/server/ReactPartialRenderer.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 254d049cc42..8b694096324 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -72,7 +72,6 @@ if (__DEV__) { var {ReactDebugCurrentFrame} = require('shared/ReactGlobalSharedState'); var currentDebugStack = null; var currentDebugElementStack = null; - var didWarnAboutNoopUpdateForComponent = {}; var setCurrentDebugStack = function(stack: Array) { var frame: Frame = stack[stack.length - 1]; currentDebugElementStack = ((frame: any): FrameDev).debugElementStack; @@ -113,6 +112,7 @@ var didWarnDefaultChecked = false; var didWarnDefaultSelectValue = false; var didWarnDefaultTextareaValue = false; var didWarnInvalidOptionChildren = false; +var didWarnAboutNoopUpdateForComponent = {}; var valuePropNames = ['value', 'defaultValue']; var newlineEatingTags = { listing: true, @@ -178,7 +178,7 @@ function warnNoop( var constructor = publicInstance.constructor; const componentName = (constructor && getComponentName(constructor)) || 'ReactClass'; - const warningKey = `${callerName}_${componentName}`; + const warningKey = `${componentName}.${callerName}`; if (didWarnAboutNoopUpdateForComponent[warningKey]) { return; } From b8c8952a1a087c27f8f23300b2aafe82ae5741fc Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 12:07:16 +0000 Subject: [PATCH 3/9] Test deduplication for noop updates in server renderer --- packages/react-dom/src/__tests__/ReactServerRendering-test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index ffae448c2d5..17494318f06 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -656,8 +656,11 @@ describe('ReactDOMServer', () => { ' This usually means you called setState() outside componentWillMount() on the server.' + ' This is a no-op.\n\nPlease check the code for the Foo component.', ); + var markup = ReactDOMServer.renderToStaticMarkup(); expect(markup).toBe('
hello
'); + jest.runOnlyPendingTimers(); + expectDev(console.error.calls.count()).toBe(1); }); it('warns with a no-op when an async forceUpdate is triggered', () => { From b576fc3c962571169aaf23afaeda5570b14c1a1d Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 12:16:02 +0000 Subject: [PATCH 4/9] Test deduplication for cWRP warning --- .../src/__tests__/ReactCompositeComponentState-test.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js index e2e04fa70fe..d0299ffc99f 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js @@ -421,6 +421,10 @@ describe('ReactCompositeComponent-state', () => { "this.state is deprecated (except inside a component's constructor). " + 'Use setState instead.', ); + + // Check deduplication + ReactDOM.render(, container); + expect(console.error.calls.count()).toEqual(1); }); it('should treat assigning to this.state inside cWM as a replaceState, with a warning', () => { From bbca76f640cf3260844ed8a9fe1fd1d9c3e6b059 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 12:19:51 +0000 Subject: [PATCH 5/9] Test deduplication for cWM setState warning --- .../react-dom/src/__tests__/ReactComponentLifeCycle-test.js | 4 ++++ packages/react/src/ReactNoopUpdateQueue.js | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js index a19e719de40..a00c065d980 100644 --- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js +++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js @@ -221,6 +221,10 @@ describe('ReactComponentLifeCycle', () => { 'unmounted component. This is a no-op.\n\nPlease check the code for the ' + 'StatefulComponent component.', ); + + // Check deduplication + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); }); it('should correctly determine if a component is mounted', () => { diff --git a/packages/react/src/ReactNoopUpdateQueue.js b/packages/react/src/ReactNoopUpdateQueue.js index 56391be1abb..3d4308af09c 100644 --- a/packages/react/src/ReactNoopUpdateQueue.js +++ b/packages/react/src/ReactNoopUpdateQueue.js @@ -18,7 +18,7 @@ function warnNoop(publicInstance, callerName) { const componentName = (constructor && (constructor.displayName || constructor.name)) || 'ReactClass'; - const warningKey = `${callerName}_${componentName}`; + const warningKey = `${componentName}.${callerName}`; if (didWarnStateUpdateForUnmountedComponent[warningKey]) { return; } From ff3d612e7d83493746b4a4b4d60fba75681f76c3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 12:21:27 +0000 Subject: [PATCH 6/9] Test deduplication for unnmounted setState warning --- .../react-dom/src/__tests__/ReactCompositeComponent-test.js | 3 +++ packages/react-reconciler/src/ReactFiberScheduler.js | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index 75f3e8060f3..7c71c2b39bf 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -252,6 +252,9 @@ describe('ReactCompositeComponent', () => { 'component. This is a no-op.\n\nPlease check the code for the ' + 'Component component.', ); + + instance.forceUpdate(); + expectDev(console.error.calls.count()).toBe(1); }); it('should warn about `setState` on unmounted components', () => { diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 21f92db452a..6a81a72e567 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -109,7 +109,6 @@ if (__DEV__) { if (didWarnStateUpdateForUnmountedComponent[componentName]) { return; } - warning( false, 'Can only update a mounted or mounting ' + From 31edad453b2f4d9d0c15e609e5f83a51cca87470 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 12:24:40 +0000 Subject: [PATCH 7/9] Fix existing Flow typing --- packages/react-reconciler/src/ReactFiberScheduler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 6a81a72e567..da3ed2bc8c0 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -120,7 +120,7 @@ if (__DEV__) { didWarnStateUpdateForUnmountedComponent[componentName] = true; }; - var warnAboutInvalidUpdates = function(instance: React$ComponentType) { + var warnAboutInvalidUpdates = function(instance: React$Component) { switch (ReactDebugCurrentFiber.phase) { case 'getChildContext': if (didWarnSetStateChildContext) { From b8e6daa28f3944e4db7ffef4882b0c6eef527202 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 12:27:19 +0000 Subject: [PATCH 8/9] Test deduplication for invalid updates --- .../src/__tests__/ReactCompositeComponent-test.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index 7c71c2b39bf..3e6da2e46d8 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -374,6 +374,11 @@ describe('ReactCompositeComponent', () => { expect(instance).toBe(instance2); expect(renderedState).toBe(1); expect(instance2.state.value).toBe(1); + + // Test deduplication + ReactDOM.unmountComponentAtNode(container); + ReactDOM.render(, container); + expectDev(console.error.calls.count()).toBe(1); }); it('should warn about `setState` in getChildContext', () => { @@ -407,6 +412,11 @@ describe('ReactCompositeComponent', () => { expectDev(console.error.calls.argsFor(0)[0]).toBe( 'Warning: setState(...): Cannot call setState() inside getChildContext()', ); + + // Test deduplication + ReactDOM.unmountComponentAtNode(container); + ReactDOM.render(, container); + expectDev(console.error.calls.count()).toBe(1); }); it('should cleanup even if render() fatals', () => { From b9369da336abf3b82e34b910034a3d06b4176b35 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 31 Oct 2017 12:31:19 +0000 Subject: [PATCH 9/9] Test deduplication of update-in-updater warning --- .../src/__tests__/ReactIncrementalUpdates-test.js | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js index 0d00233462f..b2958971995 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js @@ -341,6 +341,19 @@ describe('ReactIncrementalUpdates', () => { expect(instance.state).toEqual({a: 'a', b: 'b'}); expectDev(console.error.calls.count()).toBe(1); - console.error.calls.reset(); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'An update (setState, replaceState, or forceUpdate) was scheduled ' + + 'from inside an update function. Update functions should be pure, ' + + 'with zero side-effects. Consider using componentDidUpdate or a ' + + 'callback.', + ); + + // Test deduplication + instance.setState(function a() { + this.setState({a: 'a'}); + return {b: 'b'}; + }); + ReactNoop.flush(); + expectDev(console.error.calls.count()).toBe(1); }); });