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

fix no-unused-prop-types + async class properties and methods #1093

Merged
merged 1 commit into from Jun 28, 2017

Conversation

benstepp
Copy link
Contributor

There was an incosistency with access and storage of the usedPropTypes
of a component that would cause proptypes to not be correctly marked as
used if more than one prop was used in the body of an async function
class property or async class method.

Fixes #1053


bug source:

First time around

However, if the component tries to mark a prop as used again, it will
read from the parentComponent using utils.getParentComponent() (which in
this case will return the class) which does not have the previously used
methods. The array would then be modified (created because it doesnt
exist), and then set them onto the async arrow function overwriting
anything that was previously there.

This change just attaches used props to the found component (where the
previous usedPropTypes are pulled from) if it exists, otherwise to the
current node.

There was an inconsistency with access and storage of the usedPropTypes
of a component that would cause propTypes to not be correctly marked as
used if more than one prop was used in the body of an async function
class property or async class method.

Fixes jsx-eslint#1053

---

bug source:

First time around
  - get the parentComponent using utils.getParentComponent() https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/no-unused-prop-types.js#L594
  - save off the usedPropTypes array from what component was found.
  - modify the array with my new used props.
  - set the new array on the node https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/no-unused-prop-types.js#L638
    Note that in this case the node is a MemberExpression
  - Components#set will then just crawl the parents of the current node
  (the MemberExpresion) until one was found in the internal this._list.
  - Because all async FunctionExpressions, ArrowFunctionExpressions, and
  FunctionDeclaration are treated as components of confidence 0 (not a
  component). The unusedPropTypes would be attached
  to this. (Which is usually fine).

However, if the component tries to mark a prop as used again, it will
read from the parentComponent using utils.getParentComponent() (which in
this case will return the class) which does not have the previously used
methods. The array would then be modified (created because it doesnt
exist), and then set them onto the async arrow function overwriting
anything that was previously there.

This change just attaches used props to the found component (where the
previous usedPropTypes are pulled from) if it exists, otherwise to the
current node.

---

Squashed:
Added test cases for factory methods on classes that return async
functions.

Added test cases using the default eslint parser and defining an async
function in the render method.
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM.

Does this only happen with the combo of class properties and async functions, or are there test cases with async functions but not class properties we could add?

@benstepp
Copy link
Contributor Author

It only happens whenever an async function was defined and used props without destructuring inside of a class.

I do have a few async methods for the non class property variety. And I just added four more test cases to hit a questionable factory method that returns an async function case. Although I don't see why you'd want to do that, but it's possible.

// factory functions that return async functions
code: [
'export class Example extends Component {',
' static propTypes = {',
Copy link
Member

Choose a reason for hiding this comment

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

does this also happen if propTypes are assigned outside the class body (ie, using only stage 4 features)? if so, could we add a test case that uses the default parser instead of babel-eslint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a few more cases with async functions defined in the render method rather than returned from a separate method, and used the default eslint parser.

The issue was with marking the props as used, not with how the propTypes were defined, so using OG syntax works just fine.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks!

@vkbansal
Copy link

When will this be merged? facing the same issue.

Copy link
Collaborator

@EvHaus EvHaus left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for that fix and the detailed tests.

@TrevorSayre
Copy link

@benstepp It appears the merge conflicts still need to be handled before this can be merged

@jonandrewdavis
Copy link

Would leaving a comment here help this get merged? Having this issue and found that it's alllmost there. Keep up the good work here!

👍

Copy link
Collaborator

@lencioni lencioni left a comment

Choose a reason for hiding this comment

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

Looks like this needs a rebase

@ljharb
Copy link
Member

ljharb commented Jun 28, 2017

I took the liberty of rebasing this - it was pretty tricky. I'll merge once tests pass.

@lencioni lencioni merged commit a5dc9b8 into jsx-eslint:master Jun 28, 2017
@rkkautsar
Copy link

Having this issue and seeing the fix is already in the master but not released yet...
When is the next release scheduled?

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.

None yet

8 participants