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

Upgrade to ESLint 9 and migrate to flat config #82

Merged
merged 2 commits into from
Nov 14, 2024
Merged

Conversation

billyjanitsch
Copy link
Collaborator

Closes #74.

Notable changes:

  • In general, the config has been stripped back significantly, mostly hewing to the recommended ESLint and TS-ESLint configs.
  • Import/export checking has been left entirely to the TS compiler. This means we won't catch issues with malformed JS imports, but we essentially only use JS for config files where any issues are likely to surface obviously at build-time or runtime.
  • The React and JSX-a11y plugins have been removed entirely. (The official React Hooks plugin remains.) We may add these back with more permissive rule sets, but we weren't deriving much value from them and both packages have been suffering from some maintenance issues. I'm curious to keep an eye on the eslint-react project which may end up replacing these as it matures.
  • The JSDoc plugin has been removed. We don't use JSDoc comments often enough to warrant the complexity/overhead. We can always add this back if we miss it.

I'd like to add some additional fixtures to the test suite and continue to fiddle slightly with the config before release, but I think this is a good checkpoint to merge. I've patched this new config into two internal projects to verify that it doesn't introduce any surprising regressions.

@billyjanitsch billyjanitsch requested a review from a team as a code owner November 13, 2024 20:02
Copy link
Contributor

@nabeel- nabeel- left a comment

Choose a reason for hiding this comment

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

Just one question but overall this lgtm!

extensions: ['.ts', '.cts', '.mts', '.tsx', '.js', '.jsx'],
},
rules: {
'@typescript-eslint/no-unused-vars': ['error', {ignoreRestSiblings: true}],
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there specific use-cases you saw either in existing codebases or generally that nudged you in the direction of changing the default of ignoreRestSiblings? It seems like a reasonable override but I can also see how it might not be necessary to override at all (and one less deviation from the baseline).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we use this pattern occasionally in internal codebases to omit properties from an object, e.g.:

const {unused, ...rest} = props
return <button {...rest} />

Without this override, I don't see an ergonomic way to do this short of introducing an omit util. Do you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked offline a bit and it seems like since this is a pattern used enough internally and without an ergonomic alternative it justifies the override.

Thanks for providing the context @billyjanitsch !

@billyjanitsch billyjanitsch merged commit ba0847f into main Nov 14, 2024
2 checks passed
@billyjanitsch billyjanitsch deleted the billy/eslint-9 branch November 14, 2024 17:17
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.

Migrate to flat config
2 participants