-
Notifications
You must be signed in to change notification settings - Fork 45.7k
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
add functional components warning about legacy context api #12892
add functional components warning about legacy context api #12892
Conversation
@@ -600,6 +601,11 @@ function mountIndeterminateComponent( | |||
didWarnAboutBadClass[componentName] = true; | |||
} | |||
} | |||
|
|||
if (workInProgress.mode & StrictMode) { | |||
ReactStrictModeWarnings.recordLegacyContextWarning(workInProgress); |
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 expect Flow to error on this because we pass different number of arguments. Let's explicitly pass null
when we don't have an instance?
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 didn't error on this, but I certainly agree on yours.
fiber.type.contextTypes != null || | ||
fiber.type.childContextTypes != null | ||
fiber.type.childContextTypes != null || | ||
(instance && typeof instance.getChildContext === 'function') |
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 try to avoid checks like something &&
in terms of explicit something !== null &&
when that argument is coming from our own code (and we know it's either null
or an object).
@@ -864,7 +877,8 @@ describe('ReactStrictMode', () => { | |||
'Warning: Legacy context API has been detected within a strict-mode tree: ' + | |||
'\n in div (at **)' + | |||
'\n in Root (at **)' + | |||
'\n\nPlease update the following components: LegacyContextConsumer, LegacyContextProvider' + | |||
'\n\nPlease update the following components: ' + | |||
'FunctionalLegacyContextConsumer, LegacyContextConsumer, LegacyContextProvider' + | |||
'\n\nLearn more about this warning 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.
Can you also add a test for factory component please?
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.
Sure will do
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.
LGTM
As is mentioned by @gaearon in the previous PR, functional components should also be warned if they have
contextTypes
. Previously it only warns in class components.To reduce extraneous check, the logic is put into
mountIndeterminateComponent
.