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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

upgrade eslint to 7.x #1794

Merged
merged 4 commits into from Aug 16, 2020
Merged

upgrade eslint to 7.x #1794

merged 4 commits into from Aug 16, 2020

Conversation

tmsns
Copy link
Contributor

@tmsns tmsns commented Aug 6, 2020

This PR aims to upgrade eslint to 7.x. This will solve issues like: babel/eslint-plugin-babel#196

Some remarks:

  • babel-eslint was used to support optional chaining. This is now in the spec, so eslint support this natively. Should this update to @open-wc/eslint-config be considered as breaking? If users used eslint comments to control the babel rules, it will break their linting.
  • I marked it as draft, as I still am testing this on my own projects.
  • If possible, please test this in your projects as well, to make sure nothing is breaking. 馃槉

@tmsns tmsns marked this pull request as ready for review August 7, 2020 14:45
@tmsns
Copy link
Contributor Author

tmsns commented Aug 7, 2020

So, was able to work on it today:

  • tests are passing 馃帀
  • own projects are working correctly

Still two points open:

  • should this be a breaking issue
  • can you guys test as well?

@bennypowers
Copy link
Member

eslint/rfcs#47 may have ramifications for users, we'll need to determine how and document migration steps if necessary

@tmsns
Copy link
Contributor Author

tmsns commented Aug 10, 2020

Agreed.

Until now, I see following potential ramifications:

  • the change in plugin loading (the PR you mentioned, Benny)
  • usage of babel/* comments

It definitely feels like a major change, with breaking changes (ie removal of babel-eslint).

How do you guys want to document this? Should we add a section in the README?

BREAKING CHANGE: for a guide to migrate your code to eslint 7.x, as well as a list of breaking changes, please refer to:
- https://eslint.org/docs/user-guide/migrating-to-6.0.0
- https://eslint.org/docs/user-guide/migrating-to-7.0.0

Additionally, this change also removes the usage of `babel-eslint`. For most projects, this should work transparently. However, if your code specifically references one of its rules, you can:
- add `@babel/eslint-parser` to your devDependencies
or
- remove the references, as some of the rules have been merged into eslint
@tmsns
Copy link
Contributor Author

tmsns commented Aug 12, 2020

I've marked the initial commit as a breaking change, and also added more info how to migrate to eslint 7.x in the commit. This info will be added by Lerna in the changelog.

Please let me know if we need anything else. 馃槉 @bennypowers @LarsDenBakker

@tmsns
Copy link
Contributor Author

tmsns commented Aug 16, 2020

Hey @bennypowers, have you had some time to review the info I鈥檝e added? 馃檲 Let me know if something else is needed!

@bennypowers
Copy link
Member

I'm satisfied,

/me tosses the ball to @LarsDenBakker

@LarsDenBakker
Copy link
Member

If I understand it correctly, the changed approach by plugin loading in eslint requires users to install the plugins on their own. That's why the peer deps were added #1740. Do we need to include some instructions for users in the readme?

The other thing is that we need to update the project generator, but that's something for after this gets released.

@tmsns
Copy link
Contributor Author

tmsns commented Aug 16, 2020

Just reviewed the documentation of eslint's 7.x migration on this topic (https://eslint.org/docs/user-guide/migrating-to-7.0.0#plugin-resolution-has-been-updated) They mention that for most cases, the fact of loading plugins relatively from the entry configuration file instead of the current working dir, should not change anything. So, one might ask: what are the rare cases?
In the RFC they have an example of this (https://github.com/eslint/rfcs/blob/master/designs/2019-plugin-loading-improvement/README.md#the-plugin-uniqueness-with-cascading-config-files). Projects in a monorepo that have a separate config, while the plugins are loaded in the root, seem to be getting issues. The error message will be descriptive, however. If that's the kind of case, I think I agree most projects will not have issues, as the entry file is located in the root for most projects.

Taking this into account, I think it should be enough to just mention the migration guides like we currently do. That way, if users encounter errors, they will be able to follow the guides.

@LarsDenBakker LarsDenBakker merged commit 80e73a0 into open-wc:master Aug 16, 2020
@LarsDenBakker
Copy link
Member

Sounds good then

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

Successfully merging this pull request may close these issues.

None yet

3 participants