dangreenisrael / eslint-plugin-jest-formatting

ESLint rules for formatting test suites written for jest.
MIT License
156 stars 13 forks source link

feat: Support ESLint 7.x #89

Closed MichaelDeBoey closed 4 years ago

MichaelDeBoey commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

dangreenisrael commented 4 years ago

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

benkimpel commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Thanks, @G-Rath, you said it much better than I did. :smile: @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 commented 4 years ago

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

dimitropoulos commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

heads up that eslint-plugin-import just released: https://github.com/benmosher/eslint-plugin-import/issues/1772#issuecomment-640300344

MichaelDeBoey commented 4 years ago

@dangreenisrael I think this one can be merged 🙂

weyert commented 4 years ago

Any plans to progress this feature/pull request?

MichaelDeBoey commented 4 years ago

@weyert This one is good to merged 🙂

dangreenisrael commented 4 years ago

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!

benkimpel commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

dangreenisrael commented 4 years ago

@MichaelDeBoey v2 has been shipped.

Thanks again for taking the time to do this 😀

MichaelDeBoey commented 4 years ago

@dangreenisrael Always a pleasure to help out! 🙂