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

async instance methods broken in 6.8.0 #991

Closed
selbekk opened this issue Dec 5, 2016 · 10 comments · Fixed by singapore/lint-condo#214
Closed

async instance methods broken in 6.8.0 #991

selbekk opened this issue Dec 5, 2016 · 10 comments · Fixed by singapore/lint-condo#214

Comments

@selbekk
Copy link

selbekk commented Dec 5, 2016

We have a few async functions as instance methods in our component classes. An example is:

class SessionHandler extends Component {
  async ping() {
    try {
      await server.ping();
    } catch(e) { this.props.onLoggedOut(); }
  }
  render() {
     // Render some stuff
  }
}

In #989 you introduced a check for whether a component was async or not. This broke code like this for us - which it shouldn't. Suggest that #989 is removed or rewritten to support this kind of syntax. Worked perfectly on 6.7.1 :)

@ljharb
Copy link
Member

ljharb commented Dec 5, 2016

Can you elaborate on how it breaks? That change should only have affected component detection on functions.

@selbekk
Copy link
Author

selbekk commented Dec 5, 2016

Thanks for the lightning fast response there!

Oh sorry, of course. In the async method, the linter now think it is its own component. Therefore, I get linting errors like these:

'selectedCoverage' is missing in props validation  react/prop-types
'quantity' is missing in props validation          react/prop-types

on code like this:

class SomeClass extends Component {
  async onAddToCart({ selectedCoverage, quantity }) {
    // no jsx, just dispatcher calls etc
  }

  render() {
    return <CartWhatever onAddToCart={this.onAddToCart} />
  }
}

@ljharb
Copy link
Member

ljharb commented Dec 5, 2016

Thanks, that definitely seems like a bug. @taion mind submitting a PR to fix this?

taion added a commit to taion/eslint-plugin-react that referenced this issue Dec 5, 2016
taion added a commit to taion/eslint-plugin-react that referenced this issue Dec 5, 2016
taion added a commit to taion/eslint-plugin-react that referenced this issue Dec 5, 2016
@taion
Copy link
Contributor

taion commented Dec 5, 2016

Can't reproduce. See test cases added in #992.

@taion
Copy link
Contributor

taion commented Dec 5, 2016

And in fact the PR should be expected to do the exact opposite, since the only added cases blacklist component detection.

@selbekk
Copy link
Author

selbekk commented Dec 5, 2016

Thanks for the quick feedback guys :) I'll see if I can reproduce this outside of my application.

ljharb added a commit that referenced this issue Dec 5, 2016
@selbekk
Copy link
Author

selbekk commented Dec 9, 2016

Seems I can't reproduce this either outside of my own code. I'll close this until I have something to contribute.

@selbekk selbekk closed this as completed Dec 9, 2016
@bharley
Copy link

bharley commented Dec 22, 2016

I'm having the same issue:

class CertSettings extends React.Component {
  static propTypes = {
    onSave: React.PropTypes.func.isRequired,
    errors: React.PropTypes.array,
  };

  handleSubmit = async ({ certificate: certificates, key: keys }) => {
    if (!certificates || !certificates.length || !keys || !keys.length) {
      return;
    }

    const { onSave } = this.props;
    await onSave(crtContents, keyContents);
  };

  // ...
}

Results in the following linting errors:

'certificate' is missing in props validation  react/prop-types
'key' is missing in props validation          react/prop-types

@ljharb
Copy link
Member

ljharb commented Dec 22, 2016

@bharley does it have the same problem if you don't destructure in the signature?

(on a side note, there's zero reason you need an async function there, since you only have one await and you're not returning anything)

@bharley
Copy link

bharley commented Dec 22, 2016

@ljharb: It does not have a problem with that, no.

I trimmed down the function to show the relevant parts. It does look silly though...

You know what, that got me thinking... I played around with the code to it reproduce. This has issues:

export class Example extends React.Component {
  static propTypes = {
    certificates: React.PropTypes.array,
    onSave: React.PropTypes.func.isRequired,
    onDelete: React.PropTypes.func.isRequired,
    deleteFingerprint: React.PropTypes.string.isRequired,
  };

  handleDeleteConfirm = () => {
    const { deleteFingerprint } = this.props;
    this.props.onDelete(deleteFingerprint);
  };

  handleSubmit = async ({ certificate: certificates, key: keys }) => {
    if (!certificates || !certificates.length || !keys || !keys.length) {
      return;
    }

    const { onSave } = this.props;
    await onSave(certificates, keys);
  };
}

but this doesn't

export class Example extends React.Component {
  static propTypes = {
    certificates: React.PropTypes.array,
    onSave: React.PropTypes.func.isRequired,
    onDelete: React.PropTypes.func.isRequired,
    deleteFingerprint: React.PropTypes.string.isRequired,
  };

  handleSubmit = async ({ certificate: certificates, key: keys }) => {
    if (!certificates || !certificates.length || !keys || !keys.length) {
      return;
    }

    const { onSave } = this.props;
    await onSave(certificates, keys);
  };
}

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

Successfully merging a pull request may close this issue.

5 participants