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

feat: Support ESLint 7.x #89

Merged
merged 4 commits into from Jun 23, 2020

Conversation

MichaelDeBoey
Copy link
Contributor

@MichaelDeBoey MichaelDeBoey commented May 20, 2020

ESLint v7.0.0 is released 馃帀

(dev)Dependencies should be compatible with ESLint 7 too before we can merge this one:


BREAKING CHANGE: Requires Node@^10.12.x || 12.x

Closes #88

@dimitropoulos
Copy link
Contributor

@typescript-eslint 3.0.0 was released yesterday

https://github.com/typescript-eslint/typescript-eslint/releases/tag/v3.0.0

by the way, so that means all upstreams are updated :)

@MichaelDeBoey
Copy link
Contributor Author

@dimitropoulos eslint-plugin-import still needs to be released before this one can be merged

@dangreenisrael
Copy link
Owner

Thanks for taking this on @MichaelDeBoey! Please let me know if you need any support.

BREAKING CHANGE: Requires Node@^10.12.x || 12.x
@MichaelDeBoey MichaelDeBoey force-pushed the eslint-7 branch 2 times, most recently from c24668f to 48abbdd Compare May 24, 2020 11:41
@benkimpel
Copy link
Collaborator

@dangreenisrael since this is a breaking change we might want to take this opportunity to clean out the support for deprecated rules when we go to 2.0. what do you think?

@dimitropoulos
Copy link
Contributor

Just out of curiosity why is it a breaking change? I ask because many of the other plugins I use added eslint 7 support without needing to break. Is it because of the other updates like the parser?

@benkimpel
Copy link
Collaborator

Just out of curiosity why is it a breaking change? I ask because many of the other plugins I use added eslint 7 support without needing to break. Is it because of the other updates like the parser?

I suppose code-wise it's not a breaking change (haven't checked the parser changes yet, actually), but we're shutting down support for a set of node engines. If we release as a minor or patch then we could be breaking people's builds who are still on Node 8... could be a nasty surprise.

OTOH, if other eslint-based packages aren't classifying such a change as breaking then it's all good with me.

@G-Rath
Copy link

G-Rath commented May 26, 2020

This is a breaking change because it's changing the engines range.

Since eslint is specified as a peer dependency in plugins, users are able to choose what version of eslint they want to use (within the constraint of the peer dependency) which is what makes supporting eslint@7 by itself a non-breaking change, as users who are still on node@8 can continue to use eslint@6 and below without breaking their builds :)

Supporting eslint@7 doesn't require a change in engines, which is how eslint-plugin-jest added support for eslint@7, by just adjusting our ci to test against that version, to ensure nothing broke.

(so in summary: if you don't change the engines then it's not breaking - if you do change engines, you should always do so in a major version)

@benkimpel
Copy link
Collaborator

Thanks, @G-Rath, you said it much better than I did. 馃槃 @MichaelDeBoey @dimitropoulos We're 100% ok with whatever tinkering we might need to do with our setup to make sure it's non-breaking.

@MichaelDeBoey
Copy link
Contributor Author

The parser has updated it's Node requirements, so that's why I made it a breaking change

@dimitropoulos
Copy link
Contributor

Cool cool. Now's a great time to break what with node 8 and node 11 being put to pasture. I agree (for what little it's worth) that a break is appropriate.

@G-Rath
Copy link

G-Rath commented May 26, 2020

The parser has updated it's Node requirements

I assume you mean @typescript-eslint/parser, which doesn't need to be updated since it's a dev dependency.

I'm all for dropping support for old versions of node, but personally think it'd be nice to get eslint@7 support out the door in a non-major as that means other packages that have this plugin as a dependency don't have to rush to do majors themselves to allow their users to use eslint@7 :)

@benkimpel
Copy link
Collaborator

@MichaelDeBoey Sorry, I've been away for a bit. Is there any way I can help on your PR? @dangreenisrael any thoughts on the discussion around this PR?

@dimitropoulos
Copy link
Contributor

heads up that eslint-plugin-import just released: import-js/eslint-plugin-import#1772 (comment)

@MichaelDeBoey
Copy link
Contributor Author

@dangreenisrael I think this one can be merged 馃檪

@MichaelDeBoey MichaelDeBoey marked this pull request as ready for review June 11, 2020 10:41
@weyert
Copy link

weyert commented Jun 22, 2020

Any plans to progress this feature/pull request?

@MichaelDeBoey
Copy link
Contributor Author

@weyert This one is good to merged 馃檪

@MichaelDeBoey MichaelDeBoey mentioned this pull request Jun 23, 2020
@dangreenisrael
Copy link
Owner

Sorry about the delay. We just had a second kid and things have been an little nuts around my house. I will work to get this merged ASAP.

Thanks so much @MichaelDeBoey for taking the time and effort to do this!

Copy link
Owner

@dangreenisrael dangreenisrael left a comment

Choose a reason for hiding this comment

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

LGTM. I'm going to merge it and deal with a few other updates then publish a release soon.

@dangreenisrael dangreenisrael merged commit 9270f87 into dangreenisrael:master Jun 23, 2020
@benkimpel
Copy link
Collaborator

LGTM. I'm going to merge it and deal with a few other updates then publish a release soon.

@dangreenisrael Don't forget to bump to 2.0 for the release! :D

@MichaelDeBoey Thanks for your PR!

@MichaelDeBoey MichaelDeBoey deleted the eslint-7 branch June 23, 2020 22:33
@MichaelDeBoey
Copy link
Contributor Author

@dangreenisrael Congrats man! 馃帀
A newborn is a lot more important then adding support for ESLint 7.x, so totally understand it! 馃槈

@benkimpel Always a pleasure to help out 馃檪

@MichaelDeBoey
Copy link
Contributor Author

@dangreenisrael Let me know when this one will be released btw 馃檪

@dangreenisrael
Copy link
Owner

@MichaelDeBoey v2 has been shipped.

Thanks again for taking the time to do this 馃榾

@MichaelDeBoey
Copy link
Contributor Author

@dangreenisrael Always a pleasure to help out! 馃檪

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.

Support ESLint 7.x
6 participants