Validate props on context providers #12658

wherestheguac · 2018-04-20T03:36:28Z

Description

Prop types aren’t being checked on context providers, resulting in no dev warning in situations like the following:

References issue: #12636

Changes

Added checkPropTypes call to updateContextProvider
Added a test to make sure prop types are being checked on providers

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! 😄

pull-bot · 2018-04-20T03:46:21Z

Details of bundled changes.

Comparing: 999b656...79ea1b5

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	0.0%	609.63 KB	609.94 KB	140.53 KB	140.59 KB	UMD_DEV
react-dom.development.js	+0.1%	0.0%	594 KB	594.31 KB	136.37 KB	136.43 KB	NODE_DEV
ReactDOM-dev.js	+0.1%	0.0%	618.41 KB	618.78 KB	139.14 KB	139.19 KB	FB_WWW_DEV

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.1%	-0.2%	412.73 KB	413.04 KB	89.59 KB	89.45 KB	UMD_DEV
react-art.development.js	+0.1%	+0.1%	338.56 KB	338.87 KB	70.81 KB	70.87 KB	NODE_DEV
ReactART-dev.js	+0.1%	+0.1%	346.81 KB	347.18 KB	70.42 KB	70.47 KB	FB_WWW_DEV

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+0.1%	-0.2%	345.69 KB	346 KB	72.47 KB	72.33 KB	UMD_DEV
react-test-renderer.development.js	+0.1%	+0.1%	336.51 KB	336.83 KB	69.66 KB	69.72 KB	NODE_DEV
ReactTestRenderer-dev.js	+0.1%	+0.1%	344.98 KB	345.35 KB	69.31 KB	69.37 KB	FB_WWW_DEV

react-reconciler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-reconciler.development.js	+0.1%	+0.1%	316.9 KB	317.21 KB	65.12 KB	65.18 KB	NODE_DEV
react-reconciler-persistent.development.js	+0.1%	+0.1%	316.23 KB	316.55 KB	64.88 KB	64.94 KB	NODE_DEV

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.1%	+0.1%	456.63 KB	457 KB	97.38 KB	97.43 KB	RN_FB_DEV
ReactNativeRenderer-dev.js	+0.1%	+0.1%	456.38 KB	456.75 KB	97.32 KB	97.37 KB	RN_OSS_DEV
ReactFabric-dev.js	+0.1%	+0.1%	439.04 KB	439.42 KB	92.94 KB	92.99 KB	RN_FB_DEV
ReactFabric-dev.js	+0.1%	+0.1%	439.08 KB	439.45 KB	92.96 KB	93.01 KB	RN_OSS_DEV

Generated by 🚫 dangerJS

gaearon · 2018-04-20T12:08:20Z

packages/react-reconciler/src/ReactFiberBeginWork.js

+        workInProgress.type.propTypes,
+        newProps,
+        'prop',
+        'ReactProvider',


Let’s call this Context.Provider

gaearon · 2018-04-20T13:51:21Z

packages/react-reconciler/src/ReactFiberBeginWork.js

+        newProps,
+        'prop',
+        'Context.Provider',
+      );


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.

Ahhhh I meant to do this after I was done testing it and completely forgot, I’ll get on that tonight!

gaearon · 2018-04-20T13:52:22Z

packages/react-reconciler/src/ReactFiberBeginWork.js

@@ -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,


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.

wherestheguac · 2018-04-20T22:14:23Z

@gaearon fixed those issues 😃

gaearon · 2018-04-21T01:28:18Z

packages/react-reconciler/src/ReactFiberBeginWork.js

@@ -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) {


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.

gaearon · 2018-04-21T01:29:14Z

packages/react/src/__tests__/ReactContextValidator-test.js

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


Please include stack in the expected message. You’ll want to use ** placeholders similar to other tests that assert about the warning with stacks.

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?

@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

aweary · 2018-04-21T22:33:01Z

packages/react-reconciler/src/ReactFiberBeginWork.js

 let didWarnAboutBadClass;
 let didWarnAboutGetDerivedStateOnFunctionalComponent;
 let didWarnAboutStatelessRefs;
+let getStack = emptyFunction.thatReturns('');


getStack would only ever be called in DEV, so we can pass getCurrentFiberStackAddendum to checkPropTypes directly and avoid the getStack variable altogether.

gaearon · 2018-04-22T01:08:41Z

packages/react-reconciler/src/ReactFiberBeginWork.js

@@ -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;


Please move this right after imports so we only do this once. You can check how we do this in other files.

gaearon · 2018-04-22T12:39:33Z

Looks good, thanks!

wherestheguac added 7 commits April 19, 2018 22:53

checkPropTypes in updateContextProvider

1193a94

invalid “prop”

1184974

type not types` .. :l

2547b22

test

b596129

don’t need extra check with no spelling mistake (:

b26897e

change error message to specifically address provider

ed42cac

don’t need class, add extra render to make sure good props go through

63ef731

facebook-github-bot added the CLA Signed label Apr 20, 2018

nitpicky rename

ea50645

prettier

683fdfe

wherestheguac mentioned this pull request Apr 20, 2018

Can't validate value prop of Context Provider using PropTypes #12636

Closed

gaearon requested changes Apr 20, 2018

View reviewed changes

switch to Context.Provider

5a38c06

gaearon requested changes Apr 20, 2018

View reviewed changes

klamping mentioned this pull request Apr 20, 2018

React PR Test Report Listing klamping/react-webdriverio-tests#1

Open

add stack to warning, add extra undefined check

bdfab1f

gaearon reviewed Apr 21, 2018

View reviewed changes

separate dev check

ab53c91

aweary reviewed Apr 21, 2018

View reviewed changes

wherestheguac added 4 commits April 21, 2018 21:01

add stack to test

c500f24

more efficient

af05d8e

remove unused function

dd0ea94

prettier

c419075

gaearon reviewed Apr 22, 2018

View reviewed changes

const to top

79ea1b5

gaearon approved these changes Apr 22, 2018

View reviewed changes

gaearon merged commit 5dcf93d into facebook:master Apr 22, 2018

This was referenced Jun 9, 2018

Update yarn.lock kornicameister/korni#384

Closed

Update yarn.lock kornicameister/korni#385

Closed

Update yarn.lock kornicameister/korni#387

Closed

Update yarn.lock dropseed/jump#109

Closed

Update yarn.lock kornicameister/korni#390

Closed

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate props on context providers #12658

Validate props on context providers #12658

wherestheguac commented Apr 20, 2018 •

edited

pull-bot commented Apr 20, 2018 •

edited

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

gaearon Apr 20, 2018

wherestheguac Apr 20, 2018

gaearon Apr 20, 2018

wherestheguac Apr 20, 2018

gaearon Apr 20, 2018

wherestheguac commented Apr 20, 2018

gaearon Apr 21, 2018

gaearon Apr 21, 2018

wherestheguac Apr 21, 2018

aweary Apr 21, 2018

aweary Apr 21, 2018

gaearon Apr 22, 2018

gaearon commented Apr 22, 2018

Validate props on context providers #12658

Validate props on context providers #12658

Conversation

wherestheguac commented Apr 20, 2018 • edited

Description

Changes

Notes

pull-bot commented Apr 20, 2018 • edited

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wherestheguac commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Apr 22, 2018

wherestheguac commented Apr 20, 2018 •

edited

pull-bot commented Apr 20, 2018 •

edited