-
Notifications
You must be signed in to change notification settings - Fork 46.2k
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
Improve warning for invalid class contextType #15142
Conversation
ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: df7b87d...a123591 react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
'This can also happen due to a circular dependency, so ' + | ||
'try moving the createContext() call to a separate file.'; | ||
} else if (typeof contextType !== 'object') { | ||
addendum = ' However, it is set to ' + typeof contextType + '.'; |
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 we can add "a" here to make it a little clearer
addendum = ' However, it is set to a ' + typeof contextType + '.';
That way it reports "set to a string" instead of "set to string", which sounds better. Same goes for numbers, symbols, and functions.
} else if (contextType._context !== undefined) { | ||
// <Context.Consumer> | ||
addendum = ' Did you accidentally pass the Context.Consumer instead?'; | ||
} else { |
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.
Add another branch for arrays?
else if (Array.isArray(contextType)) {
addendum = 'However, it is set to an array.'
}
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.
Seems very unlikely to happen
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.
🤷♂️ seems like it could happen if you import the wrong value. I figured that if it happens it would be weird to see.
However, it is set to an object with keys {0, 1, 2, 3, 4, 5, 6...100000}
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.
Arrays are pretty rare as export values so I don't think it's worth handling.
didWarnAboutInvalidateContextType.add(type); | ||
|
||
let addendum = ''; | ||
if (contextType === null) { |
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.
Sometimes it is nice to be able to conditionally set it:
class Foo {
static contextType = isServerEnvironment ? null : Context;
}
This provides no way to do that while also ensuring you stay declarative.
Is null
really a common mistake? I'm guessing not since it's the only one you don't have a descriptive message for.
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.
Nope. Agree let’s omit it.
* Improve warning for invalid class contextType * Don't warn for null * Grammar
This adds extra warning for when
contextType
exists but isundefined
. This can happen due to module cycles in legacy environments (see #13969) and can be very difficult to debug.SSR and regular version are copy pasted.
I separated the DEV check outside the actual code because unlike in the prod code, we actually do want to warn for
undefined
-but-not-truthy code path.