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

Migrate to ESLint 9 #109

Merged
merged 28 commits into from
Oct 17, 2024
Merged

Migrate to ESLint 9 #109

merged 28 commits into from
Oct 17, 2024

Conversation

amannn
Copy link
Member

@amannn amannn commented Oct 4, 2024

Adds support for ESLint 9 and also addresses various issues raised over time.

Fixes #108
Fixes #107
Fixes #106
Fixes #105
Fixes #102
Fixes #101
Fixes #99

Prerelease

Details

I've also added two PRs to potentially get rid of peer dependency warnings in yarn v1:

In more recent npm versions, peer dependencies are automatically installed, therefore this doesn't pose a problem. The PRs would still help though, to avoid installing the dependencies in the first place.

'jest',
'cypress',
'vitest'
)),
Copy link
Member Author

Choose a reason for hiding this comment

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

ESLint 9 requires to import plugins and presets. To avoiding having to declare presets both as an import as well as an item within this array, I've added a (type-checked) function that imports them by name.

An important aspect of this is that individual presets only execute when they are loaded (since the typescript preset depends on the typescript dependency, as well as the tailwind preset depends on the tailwindcss dependency).

- Enable `@typescript-eslint/ban-ts-comment` (fixes #89)
- Upgrade available major versions of dependencies (`@typescript-eslint/eslint-plugin`)
- Add `molindo/tailwind`
- Enable `react/function-component-definition` to have auto fix for using functions of React components (fixes #75)
Copy link
Member Author

Choose a reason for hiding this comment

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

Prettier reindented this file.

},
"peerDependencies": {
"eslint": "^8.0.0"
"eslint": "^9.0.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

Prettier is not a strict peer dependency technically, but strongly recommended (and kind of assumed). Not listing it here gives flexibility to consumers though, e.g. to use alternative formatters like Biome or Oxc.

@@ -0,0 +1,26 @@
// @ts-check
Copy link
Member Author

Choose a reason for hiding this comment

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

I think authoring the files in JavaScript is quite nice here to avoid a build step entirely. Still, we can use TypeScript to validate the types in our modules.

names.map((name) => import(`./${name}.js`).then((mod) => mod.default))
);
return config.flat();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot 2024-10-07 at 17 20 27

@amannn amannn requested a review from symn October 7, 2024 15:25
@amannn amannn marked this pull request as ready for review October 7, 2024 15:25
CHANGELOG.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Member

@symn symn left a comment

Choose a reason for hiding this comment

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

Well done, only some questions from my side

"prettier": "^3.0.0"
},
"engines": {
"node": ">=10"
Copy link
Member

Choose a reason for hiding this comment

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

is there any restriction on node, eg only >=20? Asking for a few nodejs@16 friends :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yep, good point. I thought it might make sense to remove this.

eslint-config-molindo is flexible about Node.js, it only requires ESM support via .mjs (which is supported since Node.js 8.5).

However, ESLint 9 requires Node.js 18.8, so implicitly that is the lowest threshold. I thought removing this line might make sense, as we're essentially just repeating what ESLint has declared, and you'll already get a warning in older Node.js versions due to their engines setting.

For Node.js 16 apps, I guess it's ok to stay on ESLint 8 for the time being.

Does that sound reasonable to you?

Copy link
Member

Choose a reason for hiding this comment

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

yes totally fine - thanks for clarifying.

* @type {Array<import('eslint').Linter.Config>}
*/
export default [
{
Copy link
Member

@symn symn Oct 15, 2024

Choose a reason for hiding this comment

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

is it.only error already activated in the estlint-plugin-jest?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the no-focused-tests rule below.


/**
* @type {Array<import('eslint').Linter.Config>}
*/
Copy link
Member

Choose a reason for hiding this comment

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

same as above regarding it.only

Copy link
Member Author

Choose a reason for hiding this comment

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

See discussion above.

Copy link
Member

@symn symn left a comment

Choose a reason for hiding this comment

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

lgtm! thanks

@amannn amannn merged commit 9c0d2ce into master Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment