Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cbranch101
Copy link

@cbranch101 cbranch101 commented Dec 13, 2017

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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'},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good fix here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isDestucturedVariableDeclaration → isDestructuredVariableDeclaration

}

// Ignore Array methods
if (Array.prototype[propertyName]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question here about typeof

@MatthewHerbst
Copy link
Contributor

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

@cbranch101 out of curiosity, why?

@ljharb
Copy link
Member

ljharb commented Dec 20, 2017

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

@brokentone
Copy link

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

@ljharb
Copy link
Member

ljharb commented Jul 6, 2018

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

@davidlewallen
Copy link

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

@ljharb
Copy link
Member

ljharb commented Aug 15, 2018

@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
Copy link
Contributor

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
Copy link
Author

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

@ljharb
Copy link
Member

ljharb commented Dec 13, 2019

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

@ljharb
Copy link
Member

ljharb commented Oct 15, 2020

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

@ljharb ljharb force-pushed the master branch 2 times, most recently from 380e32c to 51d342b Compare July 4, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

'foo' is missing in props validation (react/prop-types) prop-types fails with destructuring
6 participants