-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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
Deduplicated many warnings (#11140) #11216
Conversation
const currentComponent = | ||
(constructor && (constructor.displayName || constructor.name)) || | ||
'ReactClass'; | ||
const currentComponentError = |
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
1b55c2e
to
59d751d
Compare
@gaearon Changes done. Please take a look. |
@@ -394,12 +395,13 @@ module.exports = function( | |||
if (instance.state !== oldState) { | |||
if (__DEV__) { | |||
warning( | |||
false, | |||
didWarnStateDeprecation, |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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>, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
59d751d
to
1eba12e
Compare
@gaearon Please take a look when your free :) |
There was a problem hiding this 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.
1eba12e
to
4a100f8
Compare
@gaearon Changes done :) Please take a look |
There was a problem hiding this 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 = |
There was a problem hiding this comment.
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}`; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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 = {}; |
There was a problem hiding this comment.
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 = {}; |
There was a problem hiding this comment.
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
4a100f8
to
6cb8133
Compare
@gaearon Thanks for patiently reviewing so many times :) Have made the changes suggested. |
There was a problem hiding this 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'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
6cb8133
to
f5bb22f
Compare
@gaearon Please take a look once again :) Thanks for being so patient and kind. Appreciate it a lot. |
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. |
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! |
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
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