Issue#11510: added verification check for misspelled propTypes #11524

M-ZubairAhmed · 2017-11-11T00:03:43Z

Prerequisites

Changes

Added the check if PropTypes is entered instead of propTypes in the static method of class for props types check.
Please let me know if i have to change anything

M-ZubairAhmed · 2017-11-11T00:07:38Z

solves #11510

gaearon · 2017-11-11T00:50:00Z

packages/react/src/ReactElementValidator.js

@@ -221,7 +221,14 @@ function validatePropTypes(element) {
  }
  var name = componentClass.displayName || componentClass.name;
  var propTypes = componentClass.propTypes;
-
+  if (componentClass.PropTypes !== undefined) {


We should deduplicate this warning so it doesn't warn more than once. See how we do with most other warnings.

I have added the boolean flag after observing other places where warning() was used. Also tested locally

M-ZubairAhmed · 2017-11-13T06:46:35Z

I hope this is the correct approach if not I will gladly change it.

gaearon · 2017-11-13T14:20:41Z

packages/react/src/ReactElementValidator.js

 function validatePropTypes(element) {
  var componentClass = element.type;
  if (typeof componentClass !== 'function') {
    return;
  }
  var name = componentClass.displayName || componentClass.name;
  var propTypes = componentClass.propTypes;
-
+  if (


Maybe let's put it into an else clause to the next if?

if (propTypes) { // ... } else if (componentClass.PropTypes && !propTypesMisspellWarningShown) { // ... }

I have also tested this in fixtures/dom by introducing some props and misspelling propTypes

gaearon · 2017-11-13T14:23:02Z

packages/react/src/ReactElementValidator.js

+    propTypesMisspellWarningShown = true;
+    warning(
+      false,
+      'Static propTypes method should be accessed by `.propTypes = { }` instead of `.PropTypes = { }`',


Let's reword to say:

Component %s declared `PropTypes` instead of `propTypes`. Did you misspell the property assignment?

And pass the component name name || 'Unknown' as an argument.

changes done. Thank you @gaearon for being so patient with me.

…opTypes

gaearon

We'll also need a test for this.

gaearon · 2017-11-14T10:39:26Z

packages/react/src/ReactElementValidator.js

@@ -214,18 +214,28 @@ function validateChildKeys(node, parentType) {
 *
 * @param {ReactElement} element
 */
+var propTypesMisspellWarningShown;


Can we move this up to other DEV only variables?

Also let's initialise it to false.

Added the variable declaration under if(__DEV__){ which is after import statements. Now working on test.

Should the test be added in both ReactElementValidator-test and ReactJSXElementValidator-test?

I think just one is fine.

Oh i just added test in both of them, i pushed my code. Should i remove from one ?
Also i have verified all tests were passing and ran build in my local example which was reproducing the error.
I was wondering if i should add another test it(should not show any errors is correct propTypes property assignment is passed)?

…ts files

M-ZubairAhmed · 2017-11-15T16:33:51Z

are the tests correct or should I change something?

M-ZubairAhmed · 2017-11-21T23:25:34Z

friendly ping!

gaearon · 2017-11-22T01:06:07Z

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

+    );
+
+    expectDev(console.error.calls.count()).toBe(1);
+    /*eslint-disable max-len */


Nit: instead of disabling, you can split the message into multiple lines and concatenate them. Other tests do this.

…opTypes

M-ZubairAhmed · 2017-11-22T01:55:32Z

ok i removed eslint disable and added the split in string.

…opTypes

M-ZubairAhmed · 2017-11-23T10:15:32Z

as per recent changes in master i have added DEV check to my tests. I hope it is correct?

gaearon · 2017-11-23T21:56:23Z

This mostly looks good. I think we'll want to try this internally and see if it fires any unexpected warnings at FB. (e.g. I think I saw .PropTypes used intentionally on something renderable in RN. But maybe that was in the past.)

M-ZubairAhmed · 2017-11-24T04:59:28Z

thanks do you want me to run a quick check on a react Native project?

gaearon · 2017-11-24T11:32:08Z

Would be nice.

M-ZubairAhmed · 2017-11-25T19:58:29Z

Hey @gaearon i had a chance to run the latest master build in a rn project:
There was no un usual error produced

Steps

Build latest from master branch
replaced the build version of react inside RN node modules
Added prop-types to RN project
kept track of console logs to check if anything showed up
intentionally passed PropTypes instead of propTypes
correct warning was shown as expected
reverted the propTypes declaration
no error was shown

Does this test is good?

…ook#11524) * added verification check for misspelled propTypes * added flag to check if misspelled warning was shown to developer before * added the condition to else if and improved the warning message * moved variable under dev section & initialized it to false * added test to confirm the missmatch prop type warning in both and tests files * removed eslint disable and split error into 2 lines * changed expectDev to expect in tests * added __DEV__ condition before both tests

added verification check for misspelled propTypes

ec5279a

facebook-github-bot added the CLA Signed label Nov 11, 2017

M-ZubairAhmed changed the title ~~added verification check for misspelled propTypes~~ Issue#11510: added verification check for misspelled propTypes Nov 11, 2017

gaearon requested changes Nov 11, 2017

View reviewed changes

added flag to check if misspelled warning was shown to developer before

5f39533

gaearon requested changes Nov 13, 2017

View reviewed changes

z added 2 commits November 14, 2017 13:48

added the condition to else if and improved the warning message

4eaae3d

Merge branch 'master' of github.com:facebook/react into misspelled-pr…

e297922

…opTypes

gaearon reviewed Nov 14, 2017

View reviewed changes

z added 2 commits November 14, 2017 19:12

moved variable under dev section & initialized it to false

f381561

added test to confirm the missmatch prop type warning in both and tes…

0b557d9

…ts files

gaearon reviewed Nov 22, 2017

View reviewed changes

z added 2 commits November 22, 2017 07:06

Merge branch 'master' of github.com:facebook/react into misspelled-pr…

c39a009

…opTypes

removed eslint disable and split error into 2 lines

dddcf9b

z added 3 commits November 23, 2017 14:20

Merge branch 'master' of github.com:facebook/react into misspelled-pr…

6d15d49

…opTypes

changed expectDev to expect in tests

601bec9

added __DEV__ condition before both tests

e71096a

gaearon approved these changes Nov 23, 2017

View reviewed changes

gaearon merged commit 7d27851 into facebook:master Nov 24, 2017

M-ZubairAhmed deleted the misspelled-propTypes branch November 24, 2017 05:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue#11510: added verification check for misspelled propTypes #11524

Issue#11510: added verification check for misspelled propTypes #11524

M-ZubairAhmed commented Nov 11, 2017

M-ZubairAhmed commented Nov 11, 2017

gaearon Nov 11, 2017

M-ZubairAhmed Nov 11, 2017

M-ZubairAhmed commented Nov 13, 2017

gaearon Nov 13, 2017

M-ZubairAhmed Nov 14, 2017 •

edited

Loading

gaearon Nov 13, 2017 •

edited

Loading

M-ZubairAhmed Nov 14, 2017

gaearon left a comment

gaearon Nov 14, 2017

gaearon Nov 14, 2017

M-ZubairAhmed Nov 14, 2017

M-ZubairAhmed Nov 14, 2017

gaearon Nov 14, 2017

M-ZubairAhmed Nov 14, 2017 •

edited

Loading

M-ZubairAhmed commented Nov 15, 2017

M-ZubairAhmed commented Nov 21, 2017

gaearon Nov 22, 2017

M-ZubairAhmed commented Nov 22, 2017

M-ZubairAhmed commented Nov 23, 2017

gaearon commented Nov 23, 2017

M-ZubairAhmed commented Nov 24, 2017

gaearon commented Nov 24, 2017

M-ZubairAhmed commented Nov 25, 2017

Issue#11510: added verification check for misspelled propTypes #11524

Issue#11510: added verification check for misspelled propTypes #11524

Conversation

M-ZubairAhmed commented Nov 11, 2017

Prerequisites

Changes

M-ZubairAhmed commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-ZubairAhmed commented Nov 13, 2017

Choose a reason for hiding this comment

M-ZubairAhmed Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a 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

Choose a reason for hiding this comment

M-ZubairAhmed Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

M-ZubairAhmed commented Nov 15, 2017

M-ZubairAhmed commented Nov 21, 2017

Choose a reason for hiding this comment

M-ZubairAhmed commented Nov 22, 2017

M-ZubairAhmed commented Nov 23, 2017

gaearon commented Nov 23, 2017

M-ZubairAhmed commented Nov 24, 2017

gaearon commented Nov 24, 2017

M-ZubairAhmed commented Nov 25, 2017

Steps

M-ZubairAhmed Nov 14, 2017 •

edited

Loading

gaearon Nov 13, 2017 •

edited

Loading

M-ZubairAhmed Nov 14, 2017 •

edited

Loading