-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Validate props on context providers #12658
Validate props on context providers #12658
Conversation
Details of bundled changes.Comparing: 999b656...79ea1b5 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
workInProgress.type.propTypes, | ||
newProps, | ||
'prop', | ||
'ReactProvider', |
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 Context.Provider
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.
Done!
newProps, | ||
'prop', | ||
'Context.Provider', | ||
); |
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.
This is missing the component stack argument. Can you please add it, similar to how we do elsewhere?
I think you can use getCurrentFiberStackAddendum
similar to how we do in ReactDOMFiberComponent.js
.
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.
Ahhhh I meant to do this after I was done testing it and completely forgot, I’ll get on that tonight!
@@ -885,6 +886,15 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
const newValue = newProps.value; | |||
workInProgress.memoizedProps = newProps; | |||
|
|||
if (__DEV__) { | |||
checkPropTypes( | |||
workInProgress.type.propTypes, |
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 only call checkPropTypes
if workInProgress.type.propTypes
exists. Read it before the call and then, if it exists, call the function, passing it as an argument.
@gaearon fixed those issues 😃 |
@@ -885,6 +891,18 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
const newValue = newProps.value; | |||
workInProgress.memoizedProps = newProps; | |||
|
|||
const providerPropTypes = workInProgress.type.propTypes; | |||
|
|||
if (__DEV__ && providerPropTypes) { |
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.
Could you please split this into two nested conditions? We try to always keep if (__DEV__) {
as a separate block to make it very distinctive.
ReactTestUtils.renderIntoDocument(<TestContext.Provider />), | ||
).toWarnDev( | ||
'Warning: Failed prop type: The prop `value` is marked as required in ' + | ||
'`Context.Provider`, but its value is `undefined`.', |
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.
Please include stack in the expected message. You’ll want to use **
placeholders similar to other tests that assert about the warning with stacks.
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’m having a bit of trouble finding an example of this in the tests. Potentially just not sure what I’m looking for. Do you know of one that uses this?
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.
@nicolevy you can find an example of what Dan means here: https://github.com/facebook/react/blob/master/packages/react/src/__tests__/ReactContextValidator-test.js#L182-L183
let didWarnAboutBadClass; | ||
let didWarnAboutGetDerivedStateOnFunctionalComponent; | ||
let didWarnAboutStatelessRefs; | ||
let getStack = emptyFunction.thatReturns(''); |
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.
getStack
would only ever be called in DEV, so we can pass getCurrentFiberStackAddendum
to checkPropTypes
directly and avoid the getStack
variable altogether.
@@ -885,6 +886,20 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
const newValue = newProps.value; | |||
workInProgress.memoizedProps = newProps; | |||
|
|||
if (__DEV__) { | |||
const providerPropTypes = workInProgress.type.propTypes; | |||
const {getCurrentFiberStackAddendum} = ReactDebugCurrentFiber; |
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.
Please move this right after imports so we only do this once. You can check how we do this in other files.
Looks good, thanks! |
Description
Prop types aren’t being checked on context providers, resulting in no dev warning in situations like the following:
References issue: #12636
Changes
checkPropTypes
call toupdateContextProvider
Notes
I only added one test as I thought multiple tests with different propType specification would just be testing the underlying prop-types library instead of react. If this needs to be moved or I’ve missed something let me know! 😄