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

Update eslint to v5, and eslint related plugins #2880

Closed
2 of 3 tasks
daniel-wer opened this issue Jul 11, 2018 · 3 comments · Fixed by #3063
Closed
2 of 3 tasks

Update eslint to v5, and eslint related plugins #2880

daniel-wer opened this issue Jul 11, 2018 · 3 comments · Fixed by #3063
Assignees
Milestone

Comments

@daniel-wer
Copy link
Member

daniel-wer commented Jul 11, 2018

Issues:

@daniel-wer daniel-wer self-assigned this Jul 11, 2018
@daniel-wer daniel-wer changed the title Update eslint and eslint related plugins Update eslint to v5, and eslint related plugins Jul 11, 2018
@daniel-wer
Copy link
Member Author

daniel-wer commented Jul 11, 2018

After updating eslint-config-airbnb we need to discuss whether to tweak/disable some of the new rules. @philippotto @normanrz What do you think? :)

  • import/order Could be fixed automatically according to the docs. Imo this could be helpful.
  • import/no-cycle I don't really see us fixing that.
  • react/destructuring-assignment I disabled it for now as this caused a loooot (> 500) of errors. (every time this.props.<anything> was used in any react component render method...)
  • react/no-access-state-in-setstate Around 50 errors.
  • lines-between-class-members Reasonable with the "exceptAfterSingleLine": true option imo (for declaring instance variables without a newline). Unfortunately, this errors if a flow type definition causes an instance variable declaration to have multiple lines :(
    For example eslint wants a newline after the dataLayers line in this example:
connectionInfo: ConnectionInfo;
dataLayers: {
  [key: string]: DataLayer,
};
isMappingSupported: boolean = true;
maximumDataTextureCountForLayer: number;

If you want to have a look at the linting errors yourself, check out the update-eslint branch and run yarn lint.

@normanrz
Copy link
Member

I don't have a strong opinion on the stylistic rules.
react/no-access-state-in-setstate I think might be worthwhile fixing for future proofing. Not sure if feasible everywhere.

@philippotto
Copy link
Member

import/order Could be fixed automatically according to the docs. Imo this could be helpful.

Nice 👍

import/no-cycle I don't really see us fixing that.
react/destructuring-assignment I disabled it for now as this caused a loooot (> 500) of errors. (every time this.props. was used in any react component render method...)

Agree.

react/no-access-state-in-setstate Around 50 errors.

I'd disable it for now and create a separate issue for it, so that it doesn't block the update in general. As @normanrz said, it's probably nice for future proofing.

lines-between-class-members Reasonable with the "exceptAfterSingleLine": true option imo (for declaring instance variables without a newline). Unfortunately, this errors if a flow type definition causes an instance variable declaration to have multiple lines :(
For example eslint wants a newline after the dataLayers line in this example: [...]

Let's ignore that for now, since it brings new problems.

@philippotto philippotto added this to the Sprint 26a milestone Aug 16, 2018
daniel-wer added a commit that referenced this issue Aug 16, 2018
* update eslint to latest v4 version and update latest plugins

* update eslint to v5 and update eslint-config-airbnb, disable some rules

* apply react/no-access-state-in-setstate rule (#2880)

* fix new linting errors

* update eslint related dependencies again, fix two of three remaining issues, add eslint-disable for the last issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants