Extract propTypes detection code #1911

alexzherdev · 2018-08-03T17:50:08Z

~~This still fails a bunch of tests, but wanted to put it up for preliminary high-level review early.~~

The reason tests are failing is because I extracted similar code from prop-types and no-unused-prop-types but those two have diverged and I need to reconcile two implementations into a single helper.

Next steps:

fix failing tests (this PR)
look at helper coverage when separately running tests for both rules and see if any one of them is missing test cases that they now support by virtue of using a unified implementation (PR in my fork: Additional tests for prop-types and no-unused-prop-types alexzherdev/eslint-plugin-react#1)
currently only declared propTypes detection is extracted. Could also extract used propTypes detection from prop-types and no-unused-prop-types ? (PR in my fork: Extract used prop types detection code alexzherdev/eslint-plugin-react#2)
require-default-props and default-props-match-prop-types also have some common functionality for requiredness and defaultProps detection - extract those? That would be a prerequisite to those rules using the common helper. (PRs in my fork: Extract defaultProps detection alexzherdev/eslint-plugin-react#3, Extract required propTypes detection alexzherdev/eslint-plugin-react#4)

alexzherdev · 2018-08-03T17:51:16Z

lib/rules/no-unused-prop-types.js

@@ -893,7 +454,7 @@ module.exports = {
        return;
      }

-      (props || []).forEach(prop => {
+      Object.values(props || {}).forEach(prop => {


declaredPropTypes used to be an array here, but an object in prop-types. Common implementation uses an object (keyed by prop name) for easier lookups

this should use https://npmjs.com/object.values for back compat with older nodes.

alexzherdev · 2018-08-03T17:52:43Z

lib/rules/no-unused-prop-types.js

+    }
+
+    function handleCustomValidators(component) {
+      const propTypes = component.declaredPropTypes;


This piece of code deals with used propTypes, but it was nested inside of markPropTypesAsDeclared (which was extracted out).

alexzherdev · 2018-08-03T17:54:41Z

lib/util/propTypes.js

+    const classExpressions = [];
+    const defaults = {skipShapeProps: true, customValidators: []};
+    const configuration = Object.assign({}, defaults, context.options[0] || {});
+    const customValidators = configuration.customValidators;


These two config options come from two different rules. Put them here for now, maybe will need to consider if all rules using this functionality should take both of these as options?

is there a reason the options would differ across rules? if not, maybe it makes more sense to put these in "settings"?

Actually I was partly wrong, customValidators is present in both prop-types and no-unused-prop-types. skipShapeProps comes from no-unused-prop-types only, because it can be hard to figure out that a prop access corresponds to a shape. Not sure it's needed in prop-types, will take a look.

One problem right now is skipShapeProps is true by default in no-unused-prop-types, whereas prop-types never ignores shape props. Will try to fix that by always building declaredPropTypes for shapes and checking the option when validating props.

ljharb · 2018-08-03T21:42:39Z

lib/rules/no-unused-prop-types.js

@@ -893,7 +454,7 @@ module.exports = {
        return;
      }

-      (props || []).forEach(prop => {
+      Object.values(props || {}).forEach(prop => {


this should use https://npmjs.com/object.values for back compat with older nodes.

ljharb · 2018-08-03T21:42:57Z

lib/rules/no-unused-prop-types.js

+        return;
+      }
+
+      Object.values(propTypes).forEach(propType => {


ljharb · 2018-08-03T21:43:27Z

lib/rules/no-unused-prop-types.js

+
+        if (node && (
+          node.type === 'ArrowFunctionExpression'
+          || node.type === 'FunctionExpression'


it'd be really nice if eslint exposed some kind of "is function" helper :-/ but lacking that, should we have one ourselves?

lib/util/propTypes.js

+    // Necessary because babel's scopes do not track type annotations.
+    const classExpressions = [];
+    const defaults = {skipShapeProps: true, customValidators: []};
+    const configuration = Object.assign({}, defaults, context.options[0] || {});


ljharb · 2018-08-03T21:46:17Z

lib/util/propTypes.js

+    const classExpressions = [];
+    const defaults = {skipShapeProps: true, customValidators: []};
+    const configuration = Object.assign({}, defaults, context.options[0] || {});
+    const customValidators = configuration.customValidators;


is there a reason the options would differ across rules? if not, maybe it makes more sense to put these in "settings"?

ljharb · 2018-08-03T22:21:11Z

lib/util/propTypes.js

+    const customValidators = configuration.customValidators;
+    const skipShapeProps = configuration.skipShapeProps;
+    const sourceCode = context.getSourceCode();
+    const propWrapperFunctions = new Set(context.settings.propWrapperFunctions || []);


new Set takes null and undefined quietly, so i'm not sure the || [] is needed.

ljharb · 2018-08-03T22:21:43Z

lib/util/propTypes.js

+     * @returns {Object|ASTNode} Either the whole scope or the ASTNode associated with the given identifier.
+     */
+    function typeScope(key, value) {
+      if (arguments.length === 0) {


is there another way to write this function so it doesn't use arguments?

without changing it too much, I could check for key and/or value to be undefined I guess

an alternative is creating multiple functions rather than overloading this one?

ljharb · 2018-08-03T22:22:40Z

lib/util/propTypes.js

+      }
+      seen.add(annotation);
+
+      switch (annotation.type) {


not a huge deal, but it'd be great to avoid a switch statement

hmm do you mean a map with handlers for each type? there's also a bunch of other huge switches in here (like in other parts of the codebase) which could also get the same treatment

if/else, or a map, yes

Not sure how 4-5 chained else-ifs are better than a switch? anyway will try out a map

they don't suffer from unintentional fallthrough.

alexzherdev · 2018-08-04T04:07:11Z

@ljharb thanks for the review. I'm still changing things up here fixing tests, just wanted to make sure I'm not doing anything outlandish.

alexzherdev · 2018-08-04T04:10:41Z

Github bug? 🤔(same comment listed twice, one doesn't have a reply option)

alexzherdev · 2018-08-04T21:11:24Z

@ljharb I responded to your comments. Could you also take a look at the items in the PR description to see if they make sense? If they do, should I continue modifying this PR, or would it get too big?

ljharb

I think in general your direction looks good; we should strive to separate as many changes as possible into separate PRs, land those, and then rebase this one - that way the size of the overall change won’t be a problem.

ljharb · 2018-08-05T01:27:19Z

lib/rules/display-name.js

@@ -116,7 +117,7 @@ module.exports = {
      );

      const namedFunctionExpression = (
-        (node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression') &&
+        astUtil.isFunctionLikeExpression(node) &&


I think this kind of change could be peeled off into a separate pr, and landed first?

Ah, you’re right 👍

alexzherdev · 2018-08-05T01:41:10Z

For those other changes though all of them depend on this one to be merged first.

I already have a bunch of new tests for item 2, I can push those separately then if you’d like.

I’ll split out the function helper, and then this PR can be considered complete.

alexzherdev · 2018-08-05T07:24:17Z

Split that out into #1914
Weirdly, Travis has been complaining on no-typos tests, but locally I'm getting no errors 🤔 https://travis-ci.org/yannickcr/eslint-plugin-react/jobs/412257078

alexzherdev · 2018-08-06T01:22:11Z

Rebased and the build is now passing.

alexzherdev · 2018-08-07T03:29:26Z

@ljharb by itself this PR won’t fix #1908 as it doesn’t touch that particular rule.
My comment in #1908 might be misleading though. This detection doesn’t deal with defaultProps per se, but it does have the means to handle literals as propType keys. So the broader point I was trying to make is that having centralized defaultProps handling would make things like that just work. And this PR will provide the base for that further work (if we can get other collabs to sign off on it)
Sorry for the confusion, I hope this is more clear now.

alexzherdev · 2018-08-12T00:50:42Z

@yannickcr @lencioni @EvHaus any chance you could take a look? asking because I have a truckload of sweet refactoring elsewhere that all depends on this one (see PR description) and I really like how it turned out.

ljharb · 2018-08-12T19:32:50Z

I'll merge tonight if no issues are raised.

EvHaus

Wow, thanks for doing this extraction. I was too scared to do it when I originally wrote the no-unused-prop-types rule, but it's very much needed will make sure all rules follow the same prop detection logic.

I didn't see any major issues or questions, and given that all the unit tests still pass -- I have pretty good confidence in merging this in.

alexzherdev commented Aug 3, 2018

View reviewed changes

ljharb reviewed Aug 3, 2018

View reviewed changes

ljharb reviewed Aug 5, 2018

View reviewed changes

alexzherdev force-pushed the 1674-prop-types-refactoring branch 2 times, most recently from a1d9300 to 2cfaaba Compare August 5, 2018 07:06

alexzherdev added 3 commits August 5, 2018 13:23

Extract declared propTypes detection code

f2fdaa3

Made prop-types pass

5b5ebfa

Clean up and code review

50cd5a6

alexzherdev force-pushed the 1674-prop-types-refactoring branch from 2cfaaba to 50cd5a6 Compare August 5, 2018 20:24

ljharb approved these changes Aug 6, 2018

View reviewed changes

ljharb requested review from yannickcr, lencioni and EvHaus August 6, 2018 04:36

alexzherdev mentioned this pull request Aug 6, 2018

default-props-match-prop-types doesn't detect properties in quotes #1908

Closed

EvHaus approved these changes Aug 12, 2018

View reviewed changes

ljharb merged commit c1c3d19 into jsx-eslint:master Aug 13, 2018

dependencies bot mentioned this pull request Nov 1, 2018

Update eslint-plugin-react in / from 7.4.0 to 7.11.1 leonardofaria/leozera-ui#249

Closed

This was referenced Nov 4, 2018

Update dependency eslint-plugin-react to v7.11.1 mottox2/website#11

Closed

chore(deps): update dependency eslint-plugin-react to v7 vgmtv/rsvg#12

Merged

chore(deps): update dependency eslint-plugin-react to v7.34.1 cheezenaan-sandbox/sample_app_rev4#54

Open

This was referenced Nov 23, 2018

chore(deps): update dependency eslint-plugin-react to v7 JLHwung/tianzige#31

Open

Update dependency eslint-plugin-react to v7.11.1 AugurProject/augur#109

Merged

renovate bot mentioned this pull request Dec 5, 2018

Update dependency eslint-plugin-react to v7 Automattic/jetpack#10827

Closed

1 task

mesosphere-web mentioned this pull request Dec 19, 2018

chore(deps): update dependency eslint-plugin-react to v7 mesosphere/reactjs-components#475

Closed

1 task

renovate-bot mentioned this pull request Dec 19, 2018

Update dependency eslint-plugin-react to v7.11.1 ampproject/amp-wp#1760

Merged

1 task

ljharb mentioned this pull request Jan 4, 2019

Extract duplicate code between prop-types and no-unused-prop-types #1 #1536

Closed

Extract propTypes detection code #1911

Extract propTypes detection code #1911

Conversation

alexzherdev commented Aug 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzherdev Aug 3, 2018 • edited

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

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

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

alexzherdev commented Aug 4, 2018

alexzherdev commented Aug 4, 2018

alexzherdev commented Aug 4, 2018 • edited

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzherdev commented Aug 5, 2018

alexzherdev commented Aug 5, 2018 • edited

alexzherdev commented Aug 6, 2018

alexzherdev commented Aug 7, 2018

alexzherdev commented Aug 12, 2018

ljharb commented Aug 12, 2018

EvHaus left a comment

Choose a reason for hiding this comment

alexzherdev commented Aug 3, 2018 •

edited

alexzherdev Aug 3, 2018 •

edited

alexzherdev commented Aug 4, 2018 •

edited

alexzherdev commented Aug 5, 2018 •

edited