prop-types improved handling for destructuring and member expressions #1605

cbranch101 · 2017-12-13T23:46:29Z

This adds handling for multiple previously unhandled situations related to destructuring and member expressions. It required that I pretty heavily rework the existing code related to marking prop types usages, but I think it actually simplifies it a bit from what was there before.

Multi-staged destructuring

        const Foo = ({ a: { b } }) => {
          const { c } = b
          return <div>{c}</div>
        }
// reports `'a.b.c' is missing in props validation`

Destructuring off of member expressions

        const Foo = props => {
          const { b } = props.a
          return <div>{b}</div>
        }
// reports `'a.b' is missing in props validation`

Renamed destructuring

        const Foo = ({ a: aRenamed }) => {
          const { b } = aRenamed
          return <div>{b}</div>
        }
// reports `'a.b' is missing in props validation`

Member expressions of off destructured variables

        const Foo = ({ a: { b } }) => {
          return <div>{c.d}</div>
        }
// reports `'a.b.c.d' is missing in props validation`

Everything should be working as it was before with one exception.

This test used to be reporting an error on names.map not being defined in prop type, but I'm now ignoring all array methods when checking prop types off a member expression.

{
      code: [
        'const Hello = (props) => {',
        '  let team = props.names.map((name) => {',
        '      return <li>{name}, {props.company}</li>;',
        '    });',
        '  return <ul>{team}</ul>;',
        '};'
      ].join('\n'),
      parser: 'babel-eslint',
      errors: [
        {message: '\'names\' is missing in props validation'},
        {message: '\'company\' is missing in props validation'}
      ]
    }

Fixes #1422
Fixes #1447

…ing except for MemberExpressions outside of VariableDeclarators

…ariables

…ypesAsUsed into its own function and simplified processing flow

ljharb · 2017-12-13T23:49:56Z

tests/lib/rules/prop-types.js

-        ...SharedPropTypes // eslint-disable-line object-shorthand
-      };
-    `,
+         a: React.PropTypes.string,


you've made this have 3 spaces of indentation from A.propTypes =; not that it really matters in tests, but can you revert the extra spaces?

ljharb · 2017-12-13T23:50:17Z

tests/lib/rules/prop-types.js

@@ -2445,7 +2443,6 @@ ruleTester.run('prop-types', rule, {
      parser: 'babel-eslint',
      errors: [
        {message: '\'names\' is missing in props validation'},
-        {message: '\'names.map\' is missing in props validation'},


good fix here

@ljharb Am I good on all of the changes you asked for?

ljharb · 2017-12-13T23:50:52Z

tests/lib/rules/prop-types.js

@@ -34,7 +34,6 @@ require('babel-eslint');

 const ruleTester = new RuleTester({parserOptions});
 ruleTester.run('prop-types', rule, {
-


why this change?

ljharb · 2017-12-13T23:50:54Z

lib/util/Components.js

@@ -463,6 +463,7 @@ function componentRule(rule, context) {
      let scope = context.getScope();
      while (scope) {
        const node = scope.block;
+


why this change?

ljharb · 2017-12-13T23:52:01Z

lib/rules/prop-types.js

+     * @param {ASTNode} node a VariableDeclarator node.
+     * @returns {Boolean} True if destructing, false if not.
+     */
+    function isDestucturedVariableDeclaration(node) {


isDestucturedVariableDeclaration → isDestructuredVariableDeclaration

ljharb · 2017-12-13T23:53:35Z

lib/rules/prop-types.js

+            }
+
+            // Ignore Array methods
+            if (Array.prototype[propertyName]) {


let's be a bit stricter here and check for typeof === 'function'? Also, this will include Object.prototype methods.

Also, what about when a method is shimmed/polyfilled onto Array.prototype in their code, but not in the node process that eslint is running in? I'm not sure this is a safe way to check it.

ljharb · 2017-12-13T23:53:51Z

lib/rules/prop-types.js

+          const startingNames = processedExpression.parentUsage ? processedExpression.parentUsage.allNames : [];
+          traverseMemberExpression(processedExpression.startingNode, (propertyName, currentNames, currentNode) => {
+            // Ignore Object methods
+            if (Object.prototype[propertyName] || propertyName === '__COMPUTED_PROP__') {


same question here about typeof

…rray methods are functions

MatthewHerbst · 2017-12-20T04:51:29Z

but I'm now ignoring all array methods when checking prop types off a member expression

@cbranch101 out of curiosity, why?

ljharb · 2017-12-20T05:25:36Z

@MatthewHerbst you can't know statically if .map is on an array or not.

brokentone · 2018-06-30T14:15:58Z

This rule would really help me out! Looks like the outstanding question is to @ljharb, and a conflict to be resolved.

ljharb · 2018-07-06T04:14:35Z

@cbranch101 this needs a rebase; I tried doing it but it wasn't a clean one.

davidlewallen · 2018-08-15T00:33:25Z

Any progress with this issue? Is there anything that I can do to further this along?

ljharb · 2018-08-15T05:35:07Z

@davidlewallen if you could make a branch that has these commits, rebased and passing tests, and you post a link to it here (without creating a duplicate PR) then I'd be happy to update this PR with your rebase/additions :-)

alexzherdev · 2018-08-15T07:24:00Z

I have a PR upcoming that deduplicates propType usage detection from prop-types and no-unused-prop-types alexzherdev@348021f (bringing them both down to < 200 lines compared to 1100+ lines originally 🤗). If that were landed first, then both rules would benefit from this improvement.

cbranch101 · 2018-08-15T16:19:11Z

Hey guys, sorry for the delay, got pulled off into some other things, I'll take a look at the rebase this week

ljharb · 2019-12-13T00:17:59Z

ping @cbranch101, are you still interested in completing this PR?

ljharb · 2020-10-15T18:32:23Z

@cbranch101 unfortunately it seems like @alexzherdev's propType detection refactors makes this PR not trivially rebaseable. I was able to manually preserve the test cases, but I think you'll need to be the one to look at how to rebase the implementation changes.

I'll mark this as a draft in the meantime; please unmark it when it's rebased and ready to go.

cbranch101 added 3 commits December 13, 2017 16:41

errors are now successfully reported on all valid forms of destructur…

d30eed4

…ing except for MemberExpressions outside of VariableDeclarators

errors reported on MemberExpressions off of previously destructured v…

81858de

…ariables

Removed duplicated coded, moved second switch statement in markPropsT…

a52fec4

…ypesAsUsed into its own function and simplified processing flow

ljharb requested changes Dec 13, 2017

View reviewed changes

cbranch101 added 2 commits December 13, 2017 20:29

cleaning up unnecessary changes, confirming that ignored object and a…

c92c347

…rray methods are functions

fixed linter error

aa82701

jkrawczyk mentioned this pull request Jan 16, 2019

react/prop-types - is missing in props validation - false/positive when using inner function #2135

Closed

ljharb marked this pull request as draft October 15, 2020 18:32

ljharb force-pushed the master branch from 0d1a62e to f7fe38f Compare July 11, 2022 15:02

ljharb force-pushed the master branch 6 times, most recently from 59af733 to 865ed16 Compare November 11, 2022 02:45

ljharb force-pushed the master branch 4 times, most recently from 069314a to 181c68f Compare November 18, 2022 17:19

ljharb force-pushed the master branch from 57bb8d1 to ae64aa8 Compare June 8, 2023 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prop-types improved handling for destructuring and member expressions #1605

prop-types improved handling for destructuring and member expressions #1605

cbranch101 commented Dec 13, 2017 •

edited

ljharb Dec 13, 2017

ljharb Dec 13, 2017

cbranch101 Jan 5, 2018

ljharb Dec 13, 2017

ljharb Dec 13, 2017

ljharb Dec 13, 2017

ljharb Dec 13, 2017

ljharb Dec 13, 2017

MatthewHerbst commented Dec 20, 2017

ljharb commented Dec 20, 2017

brokentone commented Jun 30, 2018

ljharb commented Jul 6, 2018

davidlewallen commented Aug 15, 2018

ljharb commented Aug 15, 2018

alexzherdev commented Aug 15, 2018

cbranch101 commented Aug 15, 2018

ljharb commented Dec 13, 2019

ljharb commented Oct 15, 2020

		@@ -34,7 +34,6 @@ require('babel-eslint');

		const ruleTester = new RuleTester({parserOptions});
		ruleTester.run('prop-types', rule, {

prop-types improved handling for destructuring and member expressions #1605

Are you sure you want to change the base?

prop-types improved handling for destructuring and member expressions #1605

Conversation

cbranch101 commented Dec 13, 2017 • edited

Multi-staged destructuring

Destructuring off of member expressions

Renamed destructuring

Member expressions of off destructured variables

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

MatthewHerbst commented Dec 20, 2017

ljharb commented Dec 20, 2017

brokentone commented Jun 30, 2018

ljharb commented Jul 6, 2018

davidlewallen commented Aug 15, 2018

ljharb commented Aug 15, 2018

alexzherdev commented Aug 15, 2018

cbranch101 commented Aug 15, 2018

ljharb commented Dec 13, 2019

ljharb commented Oct 15, 2020

cbranch101 commented Dec 13, 2017 •

edited