Upgrade to ESLint 9 #24

miyasudokoro · 2024-10-18T20:23:29Z

Migrated to ESLint 9 configurations
Updated all deprecated rules, including those that moved to @stylistic/js
Implemented TODO: do not use the built version to check the code from test/fixtures/.eslintrc.js (deleted)
Fixed the .gitattributes to actually enforce LF

commit d06d40a Merge: a462335 ebced6b Author: Devin McIntyre <k.devin.mcintyre@gmail.com> Date: Fri Oct 18 14:09:03 2024 -0600 Merge branch 'update-to-eslint-9' of https://github.com/miyasudokoro/eslint-config-jquery into update-to-eslint-9 commit a462335 Author: Devin McIntyre <devin.mcintyre@ricoh-usa.com> Date: Fri Oct 18 13:51:44 2024 -0600 update to ESLint 9 commit ebced6b Author: Devin McIntyre <devin.mcintyre@ricoh-usa.com> Date: Fri Oct 18 13:51:44 2024 -0600 update to ESLint 9

Migrated to ESLint 9 configurations Updated all deprecated rules, including those that moved to @stylistic/js Implemented TODO: do not use the built version to check the code from test/fixtures/.eslintrc.js (deleted) Fixed the .gitattributes to actually enforce LF

…/eslint-config-jquery into update-to-eslint-v9

miyasudokoro · 2024-10-18T20:43:55Z

Note: JQuery 3.7.1 has an invalid eslint ignore statement that makes the tests fail for this project in ESLint 9, but that must be
fixed in JQuery itself. The file attributes/prop.js has two "eslint ignore" statements for the same rule, which is no longer allowed. These should probably be changed to "eslint ignore next line" statements.

...\src\attributes\prop.js
113:4 error Rule "no-unused-expressions" is already configured by another configuration comment in the preceding code. This configuration is ignored

...\src\selector.js
2113:2 warning Unused eslint-enable directive (no matching eslint-disable directives were found)

mgol

Thanks for the PR, this sounds useful! I added a few remarks.

package.json

copy-fixtures.js

eslint.config.js

index.js

.gitattributes

.gitignore

copy-fixtures.js

mgol · 2024-10-18T23:30:05Z

readme.md

+export default [
+    // ... any other configurations,
+    jquery,
+    // ... any other configurations
+];


@timmywil that's probably a good idea; the way we use the config is too "picky"; we should perhaps just apply it on a global level and then provide overrides in separate blocks.

At my workplace, we use eslint-config-jquery as one of the base sets of rules, then override a handful of them that we disagree with. It looks like this:

const base = { plugins: { '@stylistic/js': stylistic, jsdoc }, rules: { 'no-unused-expressions': [ 'error', { 'allowShortCircuit': true } ], '@stylistic/js/quotes': [ 'error', 'single', { 'avoidEscape': true, 'allowTemplateLiterals': true } ], '@stylistic/js/linebreak-style': 'off', 'jsdoc/tag-lines': 0, 'jsdoc/no-defaults': 0 ..... more stuff here ..... }; ..... more stuff here .... export default [ ignores, // note: this must go first for it to be applied globally js.configs.recommended, jquery, jsdoc.configs[ 'flat/recommended-typescript-flavor' ], base, overrideForUnitTests // this has a "files" selector for unit tests and exempts them from jsdoc and max-len ];

readme.md

mgol · 2024-10-18T23:32:41Z

Note: JQuery 3.7.1 has an invalid eslint ignore statement that makes the tests fail for this project in ESLint 9

Don't worry about jQuery 3.7.1, we didn't have ESLint 9 or the flat config in the repo back then. The latest state of the 3.x-stable branch is the code for the next 3.x release and it doesn't have the comments you mentioned.

eslint.config.js

mgol

A few more comments; thanks for the changes so far!

Please re-request review once every comment is addressed; I'll do a final review pass then, including me trying it locally (I don't have enough time at the moment).

mgol · 2024-11-21T17:28:49Z

@miyasudokoro Sorry for the delay, I just wanted to give you a status update. We are focused on releasing jQuery 4.0.0 now, I'm not sure if I'll be able to look at this PR finally before we do at least the RC release - and I want to test this on a few jQuery repositories.

Also, while in jQuery on the 3.x-stable branch we use ESLint 9, on main we use ESLint 8 due to incompatibility of eslint-plugin-import with ESLint 9 & the flat config. There's active work on that done at the moment, so I hope we'll be able to upgrade soon. I'd like to merge the PR only after that happens.

mgol · 2024-11-21T17:30:22Z

BTW, the build is failing. Please update the Node.js version used in CI to 22, that should help.

miyasudokoro · 2024-12-02T18:16:35Z

@mgol Thank you for the status updates. I am not very familiar with Github workflows, but I attempted to upgrade the Node.js version.

mgol · 2024-12-02T18:50:08Z

Thanks for the Node update. The CI now fails on a different thing, can you have a look?

You don't have to wait for me approving the run, BTW - CI is mostly just doing npm install && npm test - or npm it for short - you can run it locally to find issues independently.

miyasudokoro · 2024-12-13T18:19:02Z

@mgol Those CI errors are the two things I noted in my Oct 18 comment. The tests won't pass until we're testing a jquery version that has removed those two invalid lines. I vaguely remember going to make a branch of jquery to fix them but seeing that they were already fixed by somebody else's branch, so I think this will resolve itself via the jquery 4.0.0 release.

mgol

@miyasudokoro

Those CI errors are the two things I noted in my Oct 18 comment. The tests won't pass until we're testing a jquery version that has removed those two invalid lines.

Maybe it'd be better to change the approach here. Special handling of this PR is fine since it changes so much but what if we want to introduce changes to the rules later? We would have to first fix them in jQuery source without relying on this plugin to provide them, then wait for the release and only then update them here. jQuery sometimes has long breaks between releases, this seems like too much of a churn.

I'd propose the following:

Keep test/fixtures/jquery.js in the repo.
Add an npm script that copies jQuery from devDependencies to test/fixtures/jquery.js. This task should require being invoked manually, it should not be run in CI.

Then, if we want to introduce changes to some rules, we can patch the fixture in place and release the update without waiting for the next jQuery release.

Thoughts?

mgol · 2024-12-14T12:22:00Z

package.json

  "devDependencies": {
-    "eslint": "^8.5.0"
+    "eslint": "^9.12.0",
+    "jquery": "latest"


I'd prefer to provide a concrete version here, even without the ^ range. This is because code style changes are not breaking and may happen even in patch releases, breaking the build.

mgol · 2025-03-03T17:01:59Z

@miyasudokoro Hey, are you interested in finishing this PR?

miyasudokoro added 4 commits October 18, 2024 14:13

Merge branch 'update-to-eslint-v9' of https://github.com/miyasudokoro…

b282bab

…/eslint-config-jquery into update-to-eslint-v9

fix: move note from readme to pull request

399dd1a

miyasudokoro mentioned this pull request Oct 18, 2024

Build: Enforce ECMAScript 5 in tests via ESLint jquery/jquery#5563

Merged

2 tasks

mgol requested changes Oct 18, 2024

View reviewed changes

mgol added the Needs review label Oct 18, 2024

mgol requested a review from timmywil October 18, 2024 23:31

fix: updates based on code review @mgol

4674271

mgol reviewed Oct 25, 2024

View reviewed changes

eslint.config.js Outdated Show resolved Hide resolved

mgol requested changes Oct 25, 2024

View reviewed changes

fix: code review updates for devDependencies and linting

65e6470

miyasudokoro requested a review from mgol October 25, 2024 19:20

chore: update build node version

5c13678

mgol requested changes Dec 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to ESLint 9 #24

Upgrade to ESLint 9 #24

miyasudokoro commented Oct 18, 2024

miyasudokoro commented Oct 18, 2024

mgol left a comment

mgol Oct 18, 2024

miyasudokoro Oct 23, 2024

mgol commented Oct 18, 2024

mgol left a comment

mgol commented Nov 21, 2024

mgol commented Nov 21, 2024

miyasudokoro commented Dec 2, 2024

mgol commented Dec 2, 2024

miyasudokoro commented Dec 13, 2024

mgol left a comment

mgol Dec 14, 2024

mgol commented Mar 3, 2025

Upgrade to ESLint 9 #24

Are you sure you want to change the base?

Upgrade to ESLint 9 #24

Conversation

miyasudokoro commented Oct 18, 2024

miyasudokoro commented Oct 18, 2024

mgol left a comment

Choose a reason for hiding this comment

mgol Oct 18, 2024

Choose a reason for hiding this comment

miyasudokoro Oct 23, 2024

Choose a reason for hiding this comment

mgol commented Oct 18, 2024

mgol left a comment

Choose a reason for hiding this comment

mgol commented Nov 21, 2024

mgol commented Nov 21, 2024

miyasudokoro commented Dec 2, 2024

mgol commented Dec 2, 2024

miyasudokoro commented Dec 13, 2024

mgol left a comment

Choose a reason for hiding this comment

mgol Dec 14, 2024

Choose a reason for hiding this comment

mgol commented Mar 3, 2025