-
-
Notifications
You must be signed in to change notification settings - Fork 20k
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
[Web] Update to eslint 9 #91863
[Web] Update to eslint 9 #91863
Conversation
To get the GHA checks to work, you'll need to change the - repo: https://github.com/pre-commit/mirrors-eslint
rev: v9.2.0
hooks:
- id: eslint
name: eslint
files: ^(platform/web/js|modules|misc/dist/html)/.*\.(js|html)$
args: [--fix, --no-config-lookup, --config, platform/web/eslint.config.mjs]
additional_dependencies:
- '@eslint/js@9.2.0'
- '@html-eslint/eslint-plugin@0.24.1'
- '@html-eslint/parser@0.24.1'
- '@stylistic/eslint-plugin@2.1.0'
- eslint-plugin-html@8.1.1
- eslint@9.2.0
- globals@15.2.0 Here's a diff file with the changes. Note that there's a slight discrepancy compared to using npm directly, as this appears to not properly check changes in |
This is probably because eslint 9 has problems with linting files that are in parent directories. It recognizes that
|
pre-commit already runs on the root directory though, so I don't imagine that's the issue. Given the |
Great!
What exactly is wrong with linting html files? I created a sample error file in misc/dist/html/ and eslint 9 run from the main project folder recognizes them correctly, both in html structure and embedded javascript. <!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
<title>Broken HTML</title>
</head>
<body>
<h1 unknown=>Hello, world!</h1>
<script type="text/javascript">
var foo=bar;
bar++;/*xxxx*/
</script>
</body>
</html>
|
eslint run directly seems to be fine; the issue for me came up with pre-commit specifically. But it's possible it was something off on my end, so feel free to implement the fixed hook I posted above. |
b0ae40d
to
28f2f6e
Compare
Fixed
To make eslint see the html files, you need to clear the https://github.com/pre-commit/mirrors-eslint/blob/main/.pre-commit-hooks.yaml
Unfortunately, eslint running from pre-commit on github is still reporting errors, this time about the lack of access to libraries.
|
I have no clue why it'd be failing on GHA when it's now working locally. |
Yes, I'm familiar with Chubercik's WIP on the biome formatter and linter. #91134 However, I thought that for now the eslint update would be less intrusive, from what I can see there are big changes being made to javascript files in his PR, right down to changing all single quotes to double qoutes. Anyway The only thing left to figure out is why eslint running via pre-commit on github works differently than locally. |
b9e99d7
to
5ee4030
Compare
So eslint is working locally on my machine on node local machine (no errors): {
"node": "22.1.0",
"acorn": "8.11.3",
"ada": "2.7.8",
"ares": "1.28.1",
"base64": "0.5.2",
"brotli": "1.1.0",
"cjs_module_lexer": "1.2.2",
"cldr": "45.0",
"icu": "75.1",
"llhttp": "9.2.1",
"modules": "127",
"napi": "9",
"nghttp2": "1.61.0",
"nghttp3": "0.7.0",
"ngtcp2": "1.3.0",
"openssl": "3.0.13+quic",
"simdjson": "3.8.0",
"simdutf": "5.2.4",
"tz": "2024a",
"undici": "6.13.0",
"unicode": "15.1",
"uv": "1.48.0",
"uvwasi": "0.0.20",
"v8": "12.4.254.14-node.11",
"zlib": "1.3.0.1-motley-7d77fb7"
} github (errors): {
"node": "18.20.2",
"acorn": "8.10.0",
"ada": "2.7.6",
"ares": "1.27.0",
"base64": "0.5.2",
"brotli": "1.0.9",
"cjs_module_lexer": "1.2.2",
"cldr": "44.1",
"icu": "74.2",
"llhttp": "6.1.1",
"modules": "108",
"napi": "9",
"nghttp2": "1.57.0",
"nghttp3": "0.7.0",
"ngtcp2": "0.8.1",
"openssl": "3.0.13+quic",
"simdutf": "4.0.8",
"tz": "2024a",
"undici": "5.28.4",
"unicode": "15.1",
"uv": "1.44.2",
"uvwasi": "0.0.19",
"v8": "10.2.154.26-node.36",
"zlib": "1.3.0.1-motley"
} |
Yea, I fixed it on my test repo https://github.com/patwork/workflow-test/actions/runs/9081620062/job/24955975753 but it's ugly hack. You need to npm install eslint in workflow https://github.com/patwork/workflow-test/blob/main/.github/workflows/blank.yml#L58-L68 |
It seems the error is caused by the wrong eslint being picked up. |
But somehow jsdoc is installed properly and can be run from node without problems. This is ok
And this is broken:
This also doesn't work:
Regardless of the fact that in the logs there is information about the loading of the additional_dependencies:
Maybe they're installed in
and eslint cannot find them there. |
I ran into some similar conflict issues when I tried converting to import eslint from 'eslint'; No clue if that's applicable here, but it's worth a shot I guess? |
ede0874
to
05f2009
Compare
@patwork I've switched the hook from using We might have to move the package.json to the root folder in the end (it's like a plague!). |
Good old I don't think npm fails, those are only warnings from old typescript eslint dependencies. I think eslint runs just fine. |
The |
Great! |
This "works", but it's not ideal; now anytime a js/html file is added/modified, the pre-commit hook has to perform a clean install of the node modules. |
b05e783
to
437ce34
Compare
437ce34
to
b907809
Compare
b907809
to
8367792
Compare
Tested locally, and got these two warnings/errors:
Both were fixed by tweaking the - id: eslint
name: eslint
language: node
entry: eslint
files: ^(platform/web/js/|modules/|misc/dist/html/).*\.(js|html)$
args: [--no-config-lookup, --no-warn-ignored, --config, platform/web/eslint.config.cjs]
additional_dependencies:
- '@eslint/js@^9.2.0'
- '@html-eslint/eslint-plugin@^0.24.1'
- '@html-eslint/parser@^0.24.1'
- '@stylistic/eslint-plugin@^2.1.0'
- 'eslint@^9.2.0'
- 'eslint-plugin-html@^8.1.1'
- 'globals@^15.2.0'
- 'espree@^10.0.1' |
Interesting, look like it's a bug in Anyway |
8367792
to
3668d6a
Compare
There are still |
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.
Works like a charm!
In the meantime, eslint 9.3.0 came out. |
Might be worth updating then, assuming everything would still work out of the box. |
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.
Looks good to me on the CI side.
JS changes look fine but I'm no expert so an OK from @godotengine/web would be good.
Tested locally with 9.3.0 and it's working without problems. No need to change anything because of semantic versioning in package.json:
All JS changes except one were done automatically by |
3668d6a
to
1a89ae7
Compare
Added |
Thanks! Great work 🥇 |
Thanks for your work, @patwork ! I didn't had the time to review your PR, being a little sick this week, but thanks for your work! I'll review it "post-merge", then. ;) |
I will say that it has been an interesting case of surprises in the new eslint 9+, node modules dependencies in its plugins and differences in the functioning of es modules and commonjs. 🤔 The current eslint.config works both under github actions, locally with What is missing (but I don't think it worked in the previous version either) is the ability to use eslint when working in code editors on files in directories higher up in the tree, e.g. |
Info
Referring to the draft #91771, I would like to propose an upgrade of the eslint tool to version 9+.
The update required the creation of a new configuration file, in the new flat config standard. The advantage of the new solution is a single common configuration file that automatically adapts to selected parts of the engine. The downside for the moment is that it makes it difficult to lint files that are higher up in the folder tree.
Features
eslint-disable
package.json
Requirements
Eslint 9 requires:
{ node: '^18.18.0 || ^20.9.0 || >=21.1.0' }
Notes
For proper operation, eslint must be run from the main Godot project directory. Therefore, the command
npm exec eslint .
run inplatform/web
will end with an error. The commandnpm run lint
andnpm run format
will work instead.Support for eslint plugins in editors and IDEs only works in the
platform/web/js
subdirectories.NPM
Current
package.json
:Exceptions
At this point, the following exceptions ignored by eslint-disable commands remain in the sources:
no-unused-vars
no-undef
no-eval
no-alert
no-console
no-shadow
strict