Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deduplicated many warnings (#11140) #11216

Merged
merged 9 commits into from
Oct 31, 2017

Conversation

imanushree
Copy link

@imanushree imanushree commented Oct 13, 2017

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()

@gaearon Let me know if it looks ok to you

const currentComponent =
(constructor && (constructor.displayName || constructor.name)) ||
'ReactClass';
const currentComponentError =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

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

@gaearon Changes done. Please take a look.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Author

@imanushree imanushree Oct 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

@gaearon Please take a look when your free :)

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

@gaearon Changes done :) Please take a look

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this componentName

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this warningKey

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above on naming.

@@ -72,6 +72,7 @@ if (__DEV__) {
var {ReactDebugCurrentFrame} = require('shared/ReactGlobalSharedState');
var currentDebugStack = null;
var currentDebugElementStack = null;
var errorInfo = {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this didWarnAboutNoopUpdateForComponent

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this didWarnAboutStateAssignmentForComponent

@imanushree
Copy link
Author

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

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'constructor). Use setState instead.',
getComponentName(workInProgress),
);
const componentName = getComponentName(workInProgress) || 'Component';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@gaearon gaearon Oct 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

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

@gaearon
Copy link
Collaborator

gaearon commented Oct 28, 2017

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.

@gaearon
Copy link
Collaborator

gaearon commented Oct 31, 2017

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!

@gaearon gaearon merged commit 3f1f3dc into facebook:master Oct 31, 2017
@imanushree
Copy link
Author

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 :)

Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
*  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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants