From 3f1f3dc12e72809c997d8e1078edfadd7e31bc14 Mon Sep 17 00:00:00 2001 From: Anushree Subramani Date: Tue, 31 Oct 2017 18:05:28 +0530 Subject: [PATCH] Deduplicated many warnings (#11140) (#11216) * 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 * Minor style fix * Test deduplication for noop updates in server renderer * Test deduplication for cWRP warning * Test deduplication for cWM setState warning * Test deduplication for unnmounted setState warning * Fix existing Flow typing * Test deduplication for invalid updates * Test deduplication of update-in-updater warning --- .../__tests__/ReactComponentLifeCycle-test.js | 4 +++ .../__tests__/ReactCompositeComponent-test.js | 13 ++++++++ .../ReactCompositeComponentState-test.js | 4 +++ .../__tests__/ReactServerRendering-test.js | 3 ++ .../src/server/ReactPartialRenderer.js | 11 ++++++- .../src/ReactFiberClassComponent.js | 19 +++++++---- .../src/ReactFiberScheduler.js | 32 ++++++++++++------- .../src/ReactFiberUpdateQueue.js | 7 +++- .../__tests__/ReactIncrementalUpdates-test.js | 15 ++++++++- packages/react/src/ReactNoopUpdateQueue.js | 12 +++++-- 10 files changed, 96 insertions(+), 24 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js index a19e719de4054..a00c065d980e8 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-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index 56bd6c63aba24..10b4b73c335c9 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', () => { @@ -391,6 +394,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', () => { @@ -424,6 +432,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', () => { diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js index e2e04fa70fe65..d0299ffc99f6e 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', () => { diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index ffae448c2d50e..17494318f0638 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', () => { diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 277eef8995943..f3f61ef7a1725 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -118,6 +118,7 @@ var didWarnDefaultChecked = false; var didWarnDefaultSelectValue = false; var didWarnDefaultTextareaValue = false; var didWarnInvalidOptionChildren = false; +var didWarnAboutNoopUpdateForComponent = {}; var valuePropNames = ['value', 'defaultValue']; var newlineEatingTags = { listing: true, @@ -181,6 +182,13 @@ function warnNoop( ) { if (__DEV__) { var constructor = publicInstance.constructor; + const componentName = + (constructor && getComponentName(constructor)) || 'ReactClass'; + const warningKey = `${componentName}.${callerName}`; + if (didWarnAboutNoopUpdateForComponent[warningKey]) { + return; + } + warning( false, '%s(...): Can only update a mounting component. ' + @@ -188,8 +196,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 396be01fb8ab0..2a1e9a64ee3b9 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 67a87c9a4a3f8..da3ed2bc8c002 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -101,34 +101,41 @@ if (__DEV__) { } = require('./ReactDebugFiberPerf'); var didWarnAboutStateTransition = false; + var didWarnSetStateChildContext = false; + var didWarnStateUpdateForUnmountedComponent = {}; - var warnAboutUpdateOnUnmounted = function( - instance: React$ComponentType, - ) { - const ctor = instance.constructor; + var warnAboutUpdateOnUnmounted = function(fiber: Fiber) { + const componentName = getComponentName(fiber) || 'ReactClass'; + if (didWarnStateUpdateForUnmountedComponent[componentName]) { + return; + } 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) { + var warnAboutInvalidUpdates = function(instance: React$Component) { 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 +143,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 +1237,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 1fd6fa18cb8b2..9773bdc9e8380 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-reconciler/src/__tests__/ReactIncrementalUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js index 0d00233462f4d..b295897199593 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); }); }); diff --git a/packages/react/src/ReactNoopUpdateQueue.js b/packages/react/src/ReactNoopUpdateQueue.js index 5b0631dd08617..3d4308af09c1b 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 = `${componentName}.${callerName}`; + 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; } }