Fix for #5468: Validate proptype definitions sooner #6316

troydemonbreun · 2016-03-22T04:16:01Z

Fix for #5468

Added typeCheckWarn() func and updated the oneOf/oneOfType tests
Added warning for invalid oneOf/OneOfType args

This is my first contribution to React, any feedback is appreciated. :)

jimfb · 2016-03-24T19:47:32Z

@zpao do you want to provide feedback? This PR has been idle for three days, and we should provide a response.

troydemonbreun · 2016-03-24T22:33:44Z

Thanks @jimfb !

gaearon · 2016-03-27T03:46:59Z

Please note that in the future, if you see that another PR that fixes this issue already exists, it’s best to ask the original submitter whether they intend to finish their work before starting a new PR. In this case, the implementation in #5476 was waiting for review after a discussion, but we didn’t review it timely. I closed the old PR because this one is more up to date but I didn’t feel very good about this, and at least pinging the author a couple of times would be a good idea in retrospect. No hard feelings though, and thank you for contributing 😉 . Usually PRs are linked from the issues, and I can see a link from #5468 to #5476. It’s a good idea to look through such links before jumping into implementation. Cheers!

troydemonbreun · 2016-03-27T19:24:10Z

@gaearon Oh, man, I'm sorry. I thought that PR has been rejected, and I'm new to OS contribution. In the issue thread I told @rppc to take my code (it's basically similar, not too many different ways to do it) and make his own PR. @zpao @jimfb - @rppc deserves the credit.

gaearon · 2016-03-27T20:26:00Z

Don’t worry about it! 😄
I shouldn’t have been so harsh in the previous message.

This happens a lot, and I personally did this mistake more than once. Some people take it harder so it’s usually best to try to avoid this but @rppc doesn’t mind so we’re all good here.

Cheers!

troydemonbreun · 2016-03-28T16:19:23Z

@gaearon Great, thanks!

gaearon · 2016-03-28T20:31:00Z

@zpao What do you think? If all is good it’s best we merge this before it becomes outdated like #5476 😄

gaearon · 2016-06-26T20:19:05Z

I think we should silence the proptypes warning, and make the initial warning the only one.
It’s confusing that it is displayed twice, and it doesn’t really help:

gaearon · 2016-06-26T20:20:27Z

src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js

@@ -45,6 +45,13 @@ function typeCheckPass(declaration, value) {
  expect(error).toBe(null);
 }

+function typeCheckWarn(propTypeFunc, message) { 


This helper is a little bit confusing because it pretends to be similar to typeCheckPass and typeCheckFail but has a completely different purpose and signature. Let’s just write this code explicitly inline in the corresponding test cases. Additionally, we can remove typeCheckFail calls from them because I think we only want the warning to appear once.

Added typeCheckWarn() func and updated the oneOf/oneOfType tests Added __DEV__ warning for invalid oneOf/OneOfType args

troydemonbreun · 2016-06-27T17:49:50Z

@gaearon how's this look?

(btw, I was listening to a podcast this week that interviewed you (shortly before you came to Facebook) that was discussing your next thing, and I think I may have heard a knowing tone in your voice when you said you weren't sure yet. ;-) )

gaearon · 2016-06-27T17:54:51Z

src/isomorphic/classic/types/ReactPropTypes.js

-      );
-    });
+    if (__DEV__) {
+      warning(false, 'Invalid argument supplied to oneOf, expected an instance of array.');


We would still want to return a function for consistency—it would just be a no-op.

@gaearon no-oped, or should I return false?

Let’s use emptyFunction.thatReturnsNull (if I recall correctly)

gaearon · 2016-06-27T20:03:02Z

src/isomorphic/classic/types/ReactPropTypes.js

@@ -229,6 +229,7 @@ function createEnumTypeChecker(expectedValues) {
  if (!Array.isArray(expectedValues)) {
    if (__DEV__) {
      warning(false, 'Invalid argument supplied to oneOf, expected an instance of array.');
+      return function() {};


You can use emptyFunction module for consistency.

gaearon · 2016-06-27T20:59:29Z

src/isomorphic/classic/types/ReactPropTypes.js

+      warning(false, 'Invalid argument supplied to oneOf, expected an instance of array.');
+      return emptyFunction.thatReturnsNull;
+    } else {
+      return createChainableTypeChecker(function() {


For consistency with DEV behavior, let’s return emptyFunction.thatReturnsNull here as well. This way we treat bad argument as early DEV warning, and since we can’t really check propTypes, we don’t.

Not that all these semantics really matter because nobody should be calling prop types in prod but it’s best to have them behave the same way until we explicitly disallow it.

@gaearon oh, so don't call createChainableTypeChecker as an alternate branch flow, always call warning?

@gaearon pushed my interpretation of your comments, thanks

gaearon · 2016-06-28T01:24:50Z

src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js

-        'red',
-        'Invalid argument supplied to oneOf, expected an instance of array.'
-      );
+      spyOn(console, ['error']);


Can you change this to spyOn(console, 'error') like the rest of the codebase does?

gaearon · 2016-06-29T00:30:51Z

Thank you!

troydemonbreun · 2016-06-29T15:09:43Z

Great, thanks!

* Fix for 5468: Validate proptype definitions sooner Added typeCheckWarn() func and updated the oneOf/oneOfType tests Added __DEV__ warning for invalid oneOf/OneOfType args * Suppress redundant error on warn; typeCheckWarn() removed * Return no-op * Using emptyFunction module for consistency * Remove createChainableTypeChecker() call * Adjust test to assert type check passes when warned (cherry picked from commit 6cc037b)

facebook-github-bot added GH Review: review-needed CLA Signed labels Mar 22, 2016

troydemonbreun mentioned this pull request Mar 22, 2016

Validate proptype definitions sooner #5468

Closed

gaearon mentioned this pull request Mar 27, 2016

Validating PropTypes when a class is created #5476

Closed

gaearon added this to the 15.x milestone Mar 28, 2016

zpao modified the milestones: 15.y.0, 15-next Jun 1, 2016

zpao modified the milestone: 15-next Jun 14, 2016

gaearon reviewed Jun 26, 2016
View reviewed changes

gaearon added GH Review: needs-revision and removed GH Review: review-needed labels Jun 26, 2016

Fix for 5468: Validate proptype definitions sooner

985a484

Added typeCheckWarn() func and updated the oneOf/oneOfType tests Added __DEV__ warning for invalid oneOf/OneOfType args

troydemonbreun force-pushed the r-5468 branch from ff13701 to 985a484 Compare June 27, 2016 16:14

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Jun 27, 2016

Suppress redundant error on warn; typeCheckWarn() removed

72e5ada

gaearon reviewed Jun 27, 2016
View reviewed changes

gaearon added the GH Review: needs-revision label Jun 27, 2016

gaearon removed the GH Review: review-needed label Jun 27, 2016

Return no-op

5a1f598

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jun 27, 2016

gaearon reviewed Jun 27, 2016
View reviewed changes

Using emptyFunction module for consistency

d225816

gaearon reviewed Jun 27, 2016
View reviewed changes

Remove createChainableTypeChecker() call

e1c7e55

gaearon reviewed Jun 28, 2016
View reviewed changes

Adjust test to assert type check passes when warned

1e6ab81

gaearon added GH Review: accepted and removed GH Review: review-needed labels Jun 29, 2016

gaearon added this to the 15-next milestone Jun 29, 2016

gaearon added the semver-patch label Jun 29, 2016

gaearon merged commit 6cc037b into facebook:master Jun 29, 2016

troydemonbreun deleted the r-5468 branch June 29, 2016 17:11

zpao added semver-minor and removed semver-patch labels Jul 8, 2016

zpao modified the milestones: 15-next, 15.3.0 Jul 13, 2016

renovate bot mentioned this pull request Feb 2, 2018

Update dependency react to v0.14.9 - autoclosed signavio/backbone-rel#29

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #5468: Validate proptype definitions sooner #6316

Fix for #5468: Validate proptype definitions sooner #6316

troydemonbreun commented Mar 22, 2016

jimfb commented Mar 24, 2016

troydemonbreun commented Mar 24, 2016

gaearon commented Mar 27, 2016

troydemonbreun commented Mar 27, 2016

gaearon commented Mar 27, 2016

troydemonbreun commented Mar 28, 2016

gaearon commented Mar 28, 2016

gaearon commented Jun 26, 2016

gaearon Jun 26, 2016

troydemonbreun commented Jun 27, 2016

gaearon Jun 27, 2016

troydemonbreun Jun 27, 2016 •

edited

Loading

gaearon Jun 27, 2016

gaearon Jun 27, 2016

gaearon Jun 27, 2016

troydemonbreun Jun 27, 2016 •

edited

Loading

troydemonbreun Jun 27, 2016

gaearon Jun 28, 2016

gaearon commented Jun 29, 2016

troydemonbreun commented Jun 29, 2016

Fix for #5468: Validate proptype definitions sooner #6316

Fix for #5468: Validate proptype definitions sooner #6316

Conversation

troydemonbreun commented Mar 22, 2016

jimfb commented Mar 24, 2016

troydemonbreun commented Mar 24, 2016

gaearon commented Mar 27, 2016

troydemonbreun commented Mar 27, 2016

gaearon commented Mar 27, 2016

troydemonbreun commented Mar 28, 2016

gaearon commented Mar 28, 2016

gaearon commented Jun 26, 2016

gaearon Jun 26, 2016

Choose a reason for hiding this comment

troydemonbreun commented Jun 27, 2016

gaearon Jun 27, 2016

Choose a reason for hiding this comment

troydemonbreun Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

gaearon Jun 27, 2016

Choose a reason for hiding this comment

gaearon Jun 27, 2016

Choose a reason for hiding this comment

gaearon Jun 27, 2016

Choose a reason for hiding this comment

troydemonbreun Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

troydemonbreun Jun 27, 2016

Choose a reason for hiding this comment

gaearon Jun 28, 2016

Choose a reason for hiding this comment

gaearon commented Jun 29, 2016

troydemonbreun commented Jun 29, 2016

troydemonbreun Jun 27, 2016 •

edited

Loading

troydemonbreun Jun 27, 2016 •

edited

Loading