Skip to content

Validate props on context providers #12658

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.

Sign up for GitHub

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

Merged
merged 17 commits into from
Apr 22, 2018
Merged

Validate props on context providers #12658

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
1193a94
checkPropTypes in updateContextProvider
wherestheguac Apr 20, 2018
1184974
invalid “prop”
wherestheguac Apr 20, 2018
2547b22
`type not `types` .. :l
wherestheguac Apr 20, 2018
b596129
test
wherestheguac Apr 20, 2018
b26897e
don’t need extra check with no spelling mistake (:
wherestheguac Apr 20, 2018
ed42cac
change error message to specifically address provider
wherestheguac Apr 20, 2018
63ef731
don’t need class, add extra render to make sure good props go through
wherestheguac Apr 20, 2018
ea50645
nitpicky rename
wherestheguac Apr 20, 2018
683fdfe
prettier
wherestheguac Apr 20, 2018
5a38c06
switch to `Context.Provider`
wherestheguac Apr 20, 2018
bdfab1f
add stack to warning, add extra undefined check
wherestheguac Apr 20, 2018
ab53c91
separate dev check
wherestheguac Apr 21, 2018
c500f24
add stack to test
wherestheguac Apr 22, 2018
af05d8e
more efficient
wherestheguac Apr 22, 2018
dd0ea94
remove unused function
wherestheguac Apr 22, 2018
c419075
prettier
wherestheguac Apr 22, 2018
79ea1b5
const to top
wherestheguac Apr 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import type {NewContext} from './ReactFiberNewContext';
import type {HydrationContext} from './ReactFiberHydrationContext';
import type {FiberRoot} from './ReactFiberRoot';
import type {ExpirationTime} from './ReactFiberExpirationTime';
import emptyFunction from 'fbjs/lib/emptyFunction';
import checkPropTypes from 'prop-types/checkPropTypes';

import {
IndeterminateComponent,
Expand Down Expand Up @@ -63,14 +65,18 @@ import {NoWork, Never} from './ReactFiberExpirationTime';
import {AsyncMode, StrictMode} from './ReactTypeOfMode';
import MAX_SIGNED_31_BIT_INT from './maxSigned31BitInt';

const {getCurrentFiberStackAddendum} = ReactDebugCurrentFiber;

let didWarnAboutBadClass;
let didWarnAboutGetDerivedStateOnFunctionalComponent;
let didWarnAboutStatelessRefs;
let getStack = emptyFunction.thatReturns('');
Copy link
Contributor

@aweary aweary Apr 21, 2018

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.


if (__DEV__) {
didWarnAboutBadClass = {};
didWarnAboutGetDerivedStateOnFunctionalComponent = {};
didWarnAboutStatelessRefs = {};
getStack = getCurrentFiberStackAddendum;
}

export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

@gaearon gaearon Apr 21, 2018

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.

checkPropTypes(
providerPropTypes,
newProps,
'prop',
'Context.Provider',
getStack,
);
Copy link
Collaborator

@gaearon gaearon Apr 20, 2018

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.

Copy link
Contributor Author

@wherestheguac wherestheguac Apr 20, 2018

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!

}

let changedBits: number;
if (oldProps === null) {
// Initial render
Expand Down
17 changes: 17 additions & 0 deletions packages/react/src/__tests__/ReactContextValidator-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,23 @@ describe('ReactContextValidator', () => {
ReactTestUtils.renderIntoDocument(<Component testContext={{foo: 'foo'}} />);
});

it('warns of incorrect prop types on context provider', () => {
const TestContext = React.createContext();

TestContext.Provider.propTypes = {
value: PropTypes.string.isRequired,
};

ReactTestUtils.renderIntoDocument(<TestContext.Provider value="val" />);

expect(() =>
ReactTestUtils.renderIntoDocument(<TestContext.Provider />),
).toWarnDev(
'Warning: Failed prop type: The prop `value` is marked as required in ' +
'`Context.Provider`, but its value is `undefined`.',
Copy link
Collaborator

@gaearon gaearon Apr 21, 2018

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.

Copy link
Contributor Author

@wherestheguac wherestheguac Apr 21, 2018

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?

Copy link
Contributor

@aweary aweary Apr 21, 2018

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

);
});

// TODO (bvaughn) Remove this test and the associated behavior in the future.
// It has only been added in Fiber to match the (unintentional) behavior in Stack.
it('should warn (but not error) if getChildContext method is missing', () => {
Expand Down