-
Notifications
You must be signed in to change notification settings - Fork 1
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
Switch to flat config format #609
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
@@ -1,75 +1,43 @@ | |||
# @cloudfour/eslint-plugin | |||
# @cloudfour/eslint-config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fun bit of history here — This repo is called eslint-config
and originally, the node package was called @cloudfour/eslint-config
with a version number of 0.X
.
One of Caleb's first tasks was bumping the version number to 1.0.0
.
Then he converted it to a super-plugin, and changed the package name to @cloudfour/eslint-plugin
and bumped the version number to 2.0.0
.
Since then, we've released 20+ versions, bumping whenever a dependency released a major version.
Now, I'm converting it back to the config, and reclaiming the @cloudfour/eslint-config
package name — which is still at 1.0.0, making this release 2.0.0!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After further consideration, I've restored the changelog and set the version number to 24. No sense throwing out our history just because the package name changed again.
}, | ||
}, | ||
// Override rules from recommended configs | ||
rules: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes have been made to the rules object
tsconfigRootDir: import.meta.dirname, | ||
}, | ||
}, | ||
rules: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes have been made to the rules object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll want to hide whitespace changes
"eslintconfig" | ||
], | ||
"peerDependencies": { | ||
"eslint": ">= 9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the syntax ESLint recommends, which translates to "any version of ESLint 9 or greater."
|
||
## Pull Requests | ||
|
||
If you are submitting a pull request that includes changes that will affect places where this ESLint config is installed, add a Changeset file to describe the outward-facing changes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changesets is nice, but a bit of a maintenance burden. I opted to rip it out and just use a standard release process like we do for our Stylelint config.
"name": "@cloudfour/eslint-plugin", | ||
"version": "23.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See earlier "historical" note about package name and version number
```sh | ||
npm install --save-dev @cloudfour/eslint-plugin eslint prettier | ||
``` | ||
- [`prefer-early-return`](./rules/prefer-early-return/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of surprised this isn't part of one of the plugins we extend. This is a great rule
}, | ||
], | ||
}, | ||
export default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a dumb question but, do we need this file? Can we just import this from somewhere/turn it on somehow? Maybe this one?
https://www.npmjs.com/package/@regru/eslint-plugin-prefer-early-return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? But we already had it in the repo, and for now I'd rather make fewer changes than introduce another dependency from a repo we don't know about. I'll file a followup issue to look into it, though.
Edit: #610
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates to the editorconfig. 👍🏽
This PR is effectively a complete rewrite of the repo, from an ESLint plugin to a flat ESLint shared config.
Previously, we were exporting a plugin because of a limitation of ESLint, which has since been addressed by the flat config changes. In a nutshell, the old config style required you to list all your config's plugins as peer dependencies, and require your users to install them all. Since we have eight dependencies, that would have been a burden, so we created our own plugin that would re-export those plugins as if they were our own rules, shifting the burden to our team to maintain.
Now, with the flat config, you can directly import dependencies, and everything just works, so we now have only a single peer dependency, ESLint itself. Much nicer.
In a perfect world, there would be no breaking changes. I've worked hard to keep things as close to the previous version of the config as possible. In practice, I had to update some of the dependencies to opt into their flat configs, so there may be a handful of breaking changes from those dependencies.
Testing
Followup Tasks