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

"no-unused-prop-types" inside async functions (include arrow) #1053

Closed
isnifer opened this issue Jan 31, 2017 · 3 comments
Closed

"no-unused-prop-types" inside async functions (include arrow) #1053

isnifer opened this issue Jan 31, 2017 · 3 comments

Comments

@isnifer
Copy link

isnifer commented Jan 31, 2017

Hi!
I think there is a bug with async functions.
property: "no-unused-prop-types"
parser: "babel-eslint@7.1.1"
where: "async functions"
example:

{
  code: [
    'export class Example extends Component {',
    '  static propTypes = {',
    '    failed: PropTypes.func.isRequired,',
    '    loadUserProfile: PropTypes.func.isRequired,',
    '  }',
    '  handleLogin = async () => {',
    '    await this.props.failed();',
    '    await this.props.loadUserProfile();',
    '  };',
    '}'
  ].join('\n'),
  parser: 'babel-eslint'
},
@GGAlanSmithee
Copy link

Some additional info which might help:

this will error:

onSubmit = async (e) => {
    this.setState({loggingIn: true, success: false, message: null})

    const {email, password} = this.state
    const {login, setAuthenticated} = this.props // not detected by eslint

    const {success, message} = await login(email, password)

    this.setState({loggingIn: false, success, message})
    setAuthenticated(success)

    if (success) {
        const {router, location: {state: {nextPathname} = {}}} = this.props

        setTimeout(() => {
            router.push(nextPathname || '/')
        }, 1000)
    }
}

this will not error:

onSubmit = async (e) => {
    this.setState({loggingIn: true, success: false, message: null})

    const {email, password} = this.state
    // properly detected by eslint
    const {login, setAuthenticated, router, location: {state: {nextPathname} = {}}} = this.props

    const {success, message} = await login(email, password)
    this.setState({loggingIn: false, success, message})
    setAuthenticated(success)

    if (success) {
        setTimeout(() => {
            router.push(nextPathname || '/')
        }, 1000)
    }
}

The only difference between these snippets is how this.props is destructured. In the first example, location and router props are destructured separately, and in the second example all props are accessed together. This is probably not the whole solution, since @isnifer isn't destructuring at all. But mabe you can try and see if this works;

{
  code: [
    'export class Example extends Component {',
    '  static propTypes = {',
    '    failed: PropTypes.func.isRequired,',
    '    loadUserProfile: PropTypes.func.isRequired,',
    '  }',
    '  handleLogin = async () => {',
    '    const {failed, loadUserProfile} = this.props;',
    '    await failed();',
    '    await loadUserProfile();',
    '  };',
    '}'
  ].join('\n'),
  parser: 'babel-eslint'
},

benstepp added a commit to benstepp/eslint-plugin-react that referenced this issue Feb 26, 2017
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 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.
benstepp added a commit to benstepp/eslint-plugin-react that referenced this issue Feb 26, 2017
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 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.
benstepp added a commit to benstepp/eslint-plugin-react that referenced this issue Feb 26, 2017
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 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.
christopherdro pushed a commit to async-la/eslint-plugin-react that referenced this issue Apr 24, 2017
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 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.
@TrevorSayre
Copy link

TrevorSayre commented Jun 1, 2017

export default class Users extends Component {

  static propTypes = {
    ...
    onLoad: PropTypes.func.isRequired,
  };

  async componentWillMount() {
    await Promise.all([
      this.props.onLoad(),
      this.onSearch(),
    ]);
    ...
  }

  ...

}

'onLoad' PropType is defined but prop is never used (react/no-unused-prop-types)

ljharb pushed a commit to benstepp/eslint-plugin-react that referenced this issue Jun 28, 2017
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.
@Wildhoney
Copy link

@TrevorSayre similar for me — using eslint-plugin-react@7.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants