Deduplicated many warnings (#11140) #11216

imanushree · 2017-10-13T11:21:28Z

Deduplicated the following warnings:

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
%s.componentWillReceiveProps(): Assigning directly to
this.state is deprecated (except inside a component's
constructor). Use setState instead.'
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.
setState(...): Cannot call setState() inside getChildContext()

@gaearon Let me know if it looks ok to you

gaearon · 2017-10-18T14:02:44Z

src/isomorphic/modern/class/ReactNoopUpdateQueue.js

+    const currentComponent =
+      (constructor && (constructor.displayName || constructor.name)) ||
+      'ReactClass';
+    const currentComponentError =


It feels odd to duplicate the message here. Can we just use the callerName and component name combined as a key, without the warning itself?

gaearon · 2017-10-18T14:02:50Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+    const ctor: Object = instance.constructor;
+    const currentComponent =
+      (ctor && (ctor.displayName || ctor.name)) || 'ReactClass';
+    const currentComponentErrorInfo =


imanushree · 2017-10-18T18:37:47Z

@gaearon Changes done. Please take a look.

gaearon · 2017-10-18T21:39:25Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -394,12 +395,13 @@ module.exports = function(
    if (instance.state !== oldState) {
      if (__DEV__) {
        warning(
-          false,
+          didWarnStateDeprecation,


I'd like to deduplicate this by component name. (We have a getComponentName call below that can be extracted into a variable and then used as key.)

I have changed this to getComponentName(..) || 'Component'. This is because, I was getting a flow error in this line::: !!stateDeprecationWarning[currentComponent], which said

access of computed property/element. Computed property cannot be accessed with null

Not sure if giving default value is the right fix for this.

gaearon · 2017-10-18T21:42:49Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -111,41 +111,52 @@ if (__DEV__) {
  } = require('ReactDebugFiberPerf');

  var didWarnAboutStateTransition = false;
+  var didWarnSetStateChildContext = false;
+  var ownerHasNoopWarning = {};

  var warnAboutUpdateOnUnmounted = function(
    instance: React$ComponentType<any>,


Let's change this to receive fiber instead. Then we can use getComponentName(fiber) instead of writing the code to get it manually.

When I changed it to fiber and tried, the tests failed because getComponentName(fiber) was returning null and not the component name itself for some reason. So had to revert it.

imanushree · 2017-10-28T12:32:19Z

@gaearon Please take a look when your free :)

gaearon

I think it would be a bit clearer if we just used warning(false everywhere and wrapped every warning into an extra if condition. Which defeats the purpose of that argument.. but I find it confusing in general and want to get rid of it anyway in the future.

imanushree · 2017-10-28T13:43:23Z

@gaearon Changes done :) Please take a look

gaearon

Let's tweak names (in all files), and also fix the issue introduced with a return statement

gaearon · 2017-10-28T13:47:32Z

packages/react-dom/src/server/ReactPartialRenderer.js

@@ -175,15 +176,23 @@ function warnNoop(
 ) {
  if (__DEV__) {
    var constructor = publicInstance.constructor;
+    const currentComponent =


Let's call this componentName

gaearon · 2017-10-28T13:47:46Z

packages/react-dom/src/server/ReactPartialRenderer.js

@@ -175,15 +176,23 @@ function warnNoop(
 ) {
  if (__DEV__) {
    var constructor = publicInstance.constructor;
+    const currentComponent =
+      (constructor && getComponentName(constructor)) || 'ReactClass';
+    const currentComponentError = `${callerName}_${currentComponent}`;


And this warningKey

gaearon · 2017-10-28T13:48:36Z

packages/react-reconciler/src/ReactFiberClassComponent.js

+        const currentComponent =
+          getComponentName(workInProgress) || 'Component';
+        if (stateDeprecationWarning[currentComponent]) {
+          return;


We shouldn't return here since that won't execute the code outside of DEV. Let's just wrap the warning into a condition.

gaearon · 2017-10-28T13:48:45Z

packages/react-reconciler/src/ReactFiberClassComponent.js

        warning(
          false,
          '%s.componentWillReceiveProps(): Assigning directly to ' +
            "this.state is deprecated (except inside a component's " +
            'constructor). Use setState instead.',
-          getComponentName(workInProgress),
+          currentComponent,


Same as above on naming.

gaearon · 2017-10-28T13:49:41Z

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 errorInfo = {};


Let's call this didWarnAboutNoopUpdateForComponent

gaearon · 2017-10-28T13:50:21Z

packages/react-reconciler/src/ReactFiberClassComponent.js

@@ -41,6 +41,7 @@ if (__DEV__) {
  var warning = require('fbjs/lib/warning');

  var {startPhaseTimer, stopPhaseTimer} = require('./ReactDebugFiberPerf');
+  var stateDeprecationWarning = {};


Let's call this didWarnAboutStateAssignmentForComponent

imanushree · 2017-10-28T14:47:42Z

@gaearon Thanks for patiently reviewing so many times :) Have made the changes suggested.

gaearon

Thanks for putting up with my nitpicks! A few more.

gaearon · 2017-10-28T15:10:47Z

packages/react-reconciler/src/ReactFiberClassComponent.js

-            'constructor). Use setState instead.',
-          getComponentName(workInProgress),
-        );
+        const componentName = getComponentName(workInProgress) || 'Component';


We should put this assignment inside the if block. So that we don't spend time doing it if we don't use the result.

but the if condition is based on this value. So, we need this statement here

Oops, you're right. I didn't realize this is only called when we already know we're gonna warn. Disregard my previous comment.

gaearon · 2017-10-28T15:14:10Z

packages/react-reconciler/src/ReactFiberScheduler.js

@@ -101,41 +101,54 @@ if (__DEV__) {
  } = require('./ReactDebugFiberPerf');

  var didWarnAboutStateTransition = false;
+  var didWarnSetStateChildContext = false;
+  var didWarnStateUpdateForUnmountedComponent = {};

  var warnAboutUpdateOnUnmounted = function(
    instance: React$ComponentType<any>,


Let's change this to take fiber as argument instead. Then we can use getComponentName(fiber) || 'ReactClass' instead of duplicating its logic.

I tried this and the tests were still failing. In the test output, getComponentName(fiber) was returning null (not sure why) and so instead of the expected component name, ReactClass was used as default value for component name and caused the test to fail. Not sure if I am missing something here.

Which tests were failing? I tried this on your branch and it worked:

- var warnAboutUpdateOnUnmounted = function( - instance: React$ComponentType<any>, - ) { - const ctor: Object = instance.constructor; - const componentName = - (ctor && (ctor.displayName || ctor.name)) || 'ReactClass'; + var warnAboutUpdateOnUnmounted = function(fiber) { + const componentName = getComponentName(fiber) || 'ReactClass'; if (didWarnStateUpdateForUnmountedComponent[componentName]) { return; } @@ -1242,7 +1238,7 @@ module.exports = function<T, P, I, TI, PI, C, CC, CX, PL>( } else { if (__DEV__) { if (!isErrorRecovery && fiber.tag === ClassComponent) { - warnAboutUpdateOnUnmounted(fiber.stateNode); + warnAboutUpdateOnUnmounted(fiber); } } return;

Maybe you forgot to update the callsite?

ooohh god.. yes i forgot to change it there, my bad. Sorry :(

* 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 facebook#11140

imanushree · 2017-10-28T18:26:58Z

@gaearon Please take a look once again :) Thanks for being so patient and kind. Appreciate it a lot.

gaearon · 2017-10-28T18:44:17Z

From a glance, this is looking good to me. But I would appreciate if we could test this code.

My suggestion would be to find existing tests that verify the warning is shown. Then change them to do what we warn for two times instead of one. We want to make sure we still have only one warning (which tests probably already assert). This proves deduplication works.

I think that would be a reasonable coverage.

outdated

gaearon · 2017-10-31T12:33:30Z

I want to get this in today so I added the coverage myself. Hope this is helpful for future reference. Everything worked well so congrats on getting it right without tests 😛 Thank you so much for this!

imanushree · 2017-11-03T07:00:58Z

Thanks a lot @gaearon !! Would love to continue contributing to react. Feel free to mention me if I can help out with anything at all in the future :)

* Deduplicated many warnings (facebook#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 facebook#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

facebook-github-bot added the CLA Signed label Oct 13, 2017

gaearon previously requested changes Oct 18, 2017

View reviewed changes

imanushree force-pushed the deduplicate-several-warnings branch 2 times, most recently from 1b55c2e to 59d751d Compare October 18, 2017 18:14

gaearon reviewed Oct 18, 2017

View reviewed changes

imanushree force-pushed the deduplicate-several-warnings branch from 59d751d to 1eba12e Compare October 28, 2017 12:20

gaearon reviewed Oct 28, 2017

View reviewed changes

imanushree force-pushed the deduplicate-several-warnings branch from 1eba12e to 4a100f8 Compare October 28, 2017 13:16

gaearon reviewed Oct 28, 2017

View reviewed changes

imanushree force-pushed the deduplicate-several-warnings branch from 4a100f8 to 6cb8133 Compare October 28, 2017 14:35

gaearon reviewed Oct 28, 2017

View reviewed changes

imanushree force-pushed the deduplicate-several-warnings branch from 6cb8133 to f5bb22f Compare October 28, 2017 18:10

gaearon added 8 commits October 31, 2017 12:06

Minor style fix

415a864

Test deduplication for noop updates in server renderer

b8c8952

Test deduplication for cWRP warning

b576fc3

Test deduplication for cWM setState warning

bbca76f

Test deduplication for unnmounted setState warning

ff3d612

Fix existing Flow typing

31edad4

Test deduplication for invalid updates

b8e6daa

Test deduplication of update-in-updater warning

b9369da

gaearon merged commit 3f1f3dc into facebook:master Oct 31, 2017

watadarkstar mentioned this pull request Dec 7, 2017

Select warning fires repeatedly #11795

Closed

renovate bot mentioned this pull request Feb 22, 2018

Update react monorepo packages to v16.2.0 mozilla/ensemble#28

Closed

This was referenced Mar 1, 2018

Update react-dom in / from 16.0.0 to 16.2.0 pcardune/redux-firebase-mirror#27

Open

Update react-test-renderer in / from 16.0.0 to 16.2.0 pcardune/redux-firebase-mirror#28

Open

This was referenced Mar 21, 2018

Update react to 16.3.2-abandoned sbardian/DebtTrackerAPI#15

Closed

Update react-dom to ^16.0.0-abandoned sbardian/DebtTrackerAPI#18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicated many warnings (#11140) #11216

Deduplicated many warnings (#11140) #11216

imanushree commented Oct 13, 2017 •

edited

gaearon Oct 18, 2017

gaearon Oct 18, 2017

imanushree commented Oct 18, 2017

gaearon Oct 18, 2017

imanushree Oct 28, 2017 •

edited

gaearon Oct 18, 2017

imanushree Oct 28, 2017

imanushree commented Oct 28, 2017

gaearon left a comment

imanushree commented Oct 28, 2017

gaearon left a comment

gaearon Oct 28, 2017

gaearon Oct 28, 2017

gaearon Oct 28, 2017

gaearon Oct 28, 2017

gaearon Oct 28, 2017

gaearon Oct 28, 2017

imanushree commented Oct 28, 2017

gaearon left a comment

gaearon Oct 28, 2017

imanushree Oct 28, 2017

gaearon Oct 28, 2017

gaearon Oct 28, 2017

imanushree Oct 28, 2017

gaearon Oct 28, 2017 •

edited

imanushree Oct 28, 2017

imanushree commented Oct 28, 2017

gaearon commented Oct 28, 2017 •

edited

gaearon commented Oct 31, 2017

imanushree commented Nov 3, 2017

Deduplicated many warnings (#11140) #11216

Deduplicated many warnings (#11140) #11216

Conversation

imanushree commented Oct 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imanushree commented Oct 18, 2017

Choose a reason for hiding this comment

imanushree Oct 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imanushree commented Oct 28, 2017

gaearon left a comment

Choose a reason for hiding this comment

imanushree commented Oct 28, 2017

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imanushree commented Oct 28, 2017

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Oct 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imanushree commented Oct 28, 2017

gaearon commented Oct 28, 2017 • edited

gaearon commented Oct 31, 2017

imanushree commented Nov 3, 2017

imanushree commented Oct 13, 2017 •

edited

imanushree Oct 28, 2017 •

edited

gaearon Oct 28, 2017 •

edited

gaearon commented Oct 28, 2017 •

edited