godot
godot copied to clipboard
godotengine
[Web] Update to eslint 9
Info
Referring to the draft https://github.com/godotengine/godot/pull/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
- Instead of the airbnb-base rule set, configs.all from the @eslint/js library is now used.
- Code style rules were added from the package @stylistic/eslint-plugin
- The previous rules have been reinstated, some of the new rules have been turned off because they were too strict.
- Thanks to the list of emscripten globals, it was possible to remove some of the configuration comments
eslint-disable - The common configuration file has significantly simplified the list of scripts in
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 in platform/web will end with an error. The command npm run lint and npm run fix will work instead.
Support for eslint plugins in editors and IDEs only works in the platform/web/js subdirectories.
NPM
Current package.json:
{
"name": "godot",
"private": true,
"version": "1.0.0",
"description": "Development and linting setup for Godot's Web platform code",
"author": "Godot Engine contributors",
"license": "MIT",
"scripts": {
"docs": "jsdoc --template js/jsdoc2rst/ js/engine/engine.js js/engine/config.js js/engine/features.js --destination ''",
"lint": "cd ../.. && eslint --no-config-lookup --config ./platform/web/eslint.config.mjs ./platform/web/js ./modules ./misc/dist/html",
"format": "cd ../.. && eslint --fix --no-config-lookup --config ./platform/web/eslint.config.mjs ./platform/web/js ./modules ./misc/dist/html"
},
"devDependencies": {
"@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",
"jsdoc": "^4.0.3"
}
}
Exceptions
At this point, the following exceptions ignored by eslint-disable commands remain in the sources:
no-unused-vars
./platform/web/js/engine/preloader.js:const Preloader = /** @constructor */ function () { // eslint-disable-line no-unused-vars
./platform/web/js/engine/config.js:const EngineConfig = {}; // eslint-disable-line no-unused-vars
./platform/web/js/engine/config.js:const InternalConfig = function (initConfig) { // eslint-disable-line no-unused-vars
./misc/dist/html/editor.html:function closeWelcomeModal(dontShowAgain) { // eslint-disable-line no-unused-vars
./misc/dist/html/editor.html:function clearPersistence() { // eslint-disable-line no-unused-vars
./misc/dist/html/editor.html:function closeEditor() { // eslint-disable-line no-unused-vars
no-undef
./platform/web/js/libs/library_godot_runtime.js: err.apply(null, Array.from(arguments)); // eslint-disable-line no-undef
./platform/web/js/libs/library_godot_runtime.js: out.apply(null, Array.from(arguments)); // eslint-disable-line no-undef
./misc/dist/html/service-worker.js: onClientMessage(event); // eslint-disable-line no-undef
no-eval
./platform/web/js/libs/library_godot_javascript_singleton.js: const global_eval = eval; // eslint-disable-line no-eval
./platform/web/js/libs/library_godot_javascript_singleton.js: eval_ret = eval(js_code); // eslint-disable-line no-eval
no-alert
./platform/web/js/libs/library_godot_display.js: window.alert(GodotRuntime.parseString(p_text)); // eslint-disable-line no-alert
./platform/web/js/libs/library_godot_display.js: alert('WebGL context lost, please reload the page'); // eslint-disable-line no-alert
no-console
./platform/web/js/jsdoc2rst/publish.js: console.log(d); // eslint-disable-line no-console
./platform/web/js/jsdoc2rst/publish.js: console.log(`Undefined symbol! ${d.memberof}`); // eslint-disable-line no-console
./platform/web/js/engine/config.js: console.log.apply(console, Array.from(arguments)); // eslint-disable-line no-console
./platform/web/js/engine/config.js: console.error.apply(console, Array.from(arguments)); // eslint-disable-line no-console
./misc/dist/html/service-worker.js: console.error('Network error: ', e); // eslint-disable-line no-console
no-shadow
./platform/web/js/engine/engine.js: function Engine(initConfig) { // eslint-disable-line no-shadow
strict
./platform/web/js/jsdoc2rst/publish.js:/* eslint-disable strict */
To get the GHA checks to work, you'll need to change the pre-commit-config.yaml eslint hook to the following:
- 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/[email protected]'
- '@html-eslint/[email protected]'
- '@html-eslint/[email protected]'
- '@stylistic/[email protected]'
- [email protected]
- [email protected]
- [email protected]
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 .html files for some reason, despite the prior pre-commit implementation handling them just fine. Still, the .js files handle as expected, so it's preferable to failing outright like it is currently.
This is probably because eslint 9 has problems with linting files that are in parent directories. It recognizes that baseDirectory is CWD and ignores anything above it. That's why the npm script looks like this:
"lint": "cd ../.. && eslint --no-config-lookup --config ./platform/web/eslint.config.mjs ./platform/web/js ./modules ./misc/dist/html",
pre-commit already runs on the root directory though, so I don't imagine that's the issue. Given the .js files seem to work fine, it's possible the .html settings are awry somehow. Or eslint 9 is borked idk
pre-commit already runs on the root directory though
Great!
it's possible the
.htmlsettings are awry somehow. Or eslint 9 is borked idk
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 --no-config-lookup --config ./platform/web/eslint.config.mjs ./misc/dist/html
misc/dist/html/_err.html
2:1 error Missing `lang` attribute in `<html>` tag @html-eslint/require-lang
8:14 error Unexpected space after attribute @html-eslint/no-extra-spacing-attrs
10:4 error Unexpected var, use let or const instead no-var
10:8 error 'foo' is assigned a value but never used no-unused-vars
10:11 error Operator '=' must be spaced @stylistic/space-infix-ops
10:12 error 'bar' is not defined no-undef
11:4 error 'bar' is not defined no-undef
11:10 error Expected exception block, space or tab after '/*' in comment @stylistic/spaced-comment
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.
Fixed jsdoc in .pre-commit-config.yaml - all three files from the engine directory must be specified in the parameters at the same time, and pre-commit adds the names of only those that have changed
args: [--template, platform/web/js/jsdoc2rst/, platform/web/js/engine/engine.js, platform/web/js/engine/config.js, platform/web/js/engine/features.js, --destination, '', -d, dry-run]
pass_filenames: false
To make eslint see the html files, you need to clear the types array from the template and add types_or.
https://github.com/pre-commit/mirrors-eslint/blob/main/.pre-commit-hooks.yaml
types: []
types_or: [javascript, html]
Unfortunately, eslint running from pre-commit on github is still reporting errors, this time about the lack of access to libraries.
[INFO] Initializing environment for [https://github.com/pre-commit/mirrors-eslint:@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.](https://github.com/pre-commit/mirrors-eslint:@eslint/js@%5E9.2.0,@html-eslint/eslint-plugin@%5E0.24.1,@html-eslint/parser@%5E0.24.1,@stylistic/eslint-plugin@%5E2.1.0,eslint@%5E9.2.0,eslint-plugin-html@%5E8.1.1,globals@%5E15.2.0.)
[...]
ESLint: 9.2.0
Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'globals' imported from /home/runner/work/godot/godot/platform/web/eslint.config.mjs
Did you mean to import globals/index.js?
Note that similar to what https://github.com/godotengine/godot/pull/90457 does for Black, Biome could replace both ESLint and Prettier with a faster tool.
Yes, I'm familiar with Chubercik's WIP on the biome formatter and linter. https://github.com/godotengine/godot/pull/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.
So eslint is working locally on my machine on node 22.1.0. On the other hand, github runs the scripts in node 18.20.2, maybe this is the cause of the errors? Who decides in which version of node the pre-commit scripts are run?
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
https://stackoverflow.com/questions/77253791/how-to-get-new-style-eslint-config-working-with-pre-commit
It seems the error is caused by the wrong eslint being picked up.
I have the same problem if I delete the node_modules folder inside platform/web with pre-commit.
I think it's trying to import from the node_modules in platform/web/ instead of the "pre-commit"-generated one (which doesn't exists, because we never run npm ci in there).
But somehow jsdoc is installed properly and can be run from node without problems.
This is ok
- id: jsdoc
name: jsdoc
language: node
entry: jsdoc
files: ^platform/web/js/engine/(engine|config|features)\.js$
args: [--template, platform/web/js/jsdoc2rst/, --destination, '', -d, dry-run]
additional_dependencies: ["[email protected]"]
And this is broken:
- repo: https://github.com/pre-commit/mirrors-eslint
rev: v9.2.0
hooks:
- id: eslint
name: eslint
files: ^(here/|other/|some/).*\.(js|html)$
types: []
types_or: [javascript, html]
args: [--no-config-lookup, --config, here/is/config/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@^9.2.0'
- 'eslint-plugin-html@^8.1.1'
- 'globals@^15.2.0'
This also doesn't work:
- repo: local
hooks:
- id: locallint
name: locallint
language: node
entry: eslint
files: ^(here/|other/|some/).*\.(js|html)$
args: [--no-config-lookup, --config, here/is/config/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@^9.2.0'
- 'eslint-plugin-html@^8.1.1'
- 'globals@^15.2.0'
Regardless of the fact that in the logs there is information about the loading of the additional_dependencies:
[INFO] Initializing environment for https://github.com/pre-commit/mirrors-eslint.
[INFO] Initializing environment for [https://github.com/pre-commit/mirrors-eslint:@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.](https://github.com/pre-commit/mirrors-eslint:@eslint/js@%5E9.2.0,@html-eslint/eslint-plugin@%5E0.24.1,@html-eslint/parser@%5E0.24.1,@stylistic/eslint-plugin@%5E2.1.0,eslint@%5E9.2.0,eslint-plugin-html@%5E8.1.1,globals@%5E15.2.0.)
[INFO] Initializing environment for local:@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.
Maybe they're installed in
"NODE_PATH": "/home/runner/.cache/pre-commit/repo1g_wmqch/node_env-system/lib/node_modules",
and eslint cannot find them there.
I ran into some similar conflict issues when I tried converting to biome, and the "solution" was to add this to the top of platform/web/eslint.config.mjs:
import eslint from 'eslint';
No clue if that's applicable here, but it's worth a shot I guess?
@patwork I've switched the hook from using pre-commit-eslint to use system and run npm manually, but now it seems to pass even when npm fails...
We might have to move the package.json to the root folder in the end (it's like a plague!).
Good old npm run lint 😄 But doesn't this way all js and html files will be linted every time? With pre-commit only changed files are checked.
I don't think npm fails, those are only warnings from old typescript eslint dependencies. I think eslint runs just fine.
I don't think npm fails, those are only warnings from old typescript eslint dependencies. I think eslint runs just fine.
The && part was being ignored, I've added another commit, which runs the npm ci separately. Assuming the hooks in the list are run in sequence, this will work (and it does seem to be the case), and correctly errors out when linting fails.
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.
Doesn't it always creating whole testing environment from scratch?
Prepare workflow directory
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v4' (SHA:0ad4b8fadaa221de15dcec353f45205ec38ea70b)
Download action repository 'awalsh128/cache-apt-pkgs-action@latest' (SHA:a6c3917cc929dd0345bfb2d3feaf9101823370ad)
Download action repository 'pre-commit/[email protected]' (SHA:2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd)
Getting action download info
Download action repository 'actions/cache@v4' (SHA:0c45773b623bea8c8e75f6c82b208c3cf94ea4f9)
Download action repository 'actions/upload-artifact@v3' (SHA:a8a3f3ad30e3422c9c7b888a15615d19a852ae32)
etc
Not for the hook it doesn't; the actual pre-commit action that happens locally: https://pre-commit.com/#quick-start
This is so frustrating, everyone is saying "this is working as intended", yet it's broken. :thinking:
I initially found the issue on pre-commit. https://github.com/pre-commit/pre-commit/issues/3022 The answer was "pre-commit just calls eslint there's no magic happening".
I later checked the pre-commit documentation, according to them for jobs with node package.json is required.
https://pre-commit.com/#node
The hook repository must have a package.json. It will be installed via npm install .. The installed package will provide an executable that will match the entry – usually through bin in package.json.
Naturally, temporarily setting up package.json did not fix anything. The modules install themselves from node.py to the pre-commit cache directory but then node does not see them.
I finally started looking for the problem in the node itself, after all, the NODE_PATH is set by what I saw in process.env. I found the answer in https://github.com/nodejs/node/issues/38687
This was done intentionally IIRC, and is documented here: https://nodejs.org/api/esm.html#esm_no_node_path Closing as there's no bug here, but feel free to continue the discussion or reopen if you think the documentation can be improved.
https://nodejs.org/api/esm.html#no-node_path
No NODE_PATH NODE_PATH is not part of resolving import specifiers. Please use symlinks if this behavior is desired.
:clown_face:
This is it, the solution to the problem. In order for the flat config from eslint to work correctly in a pre-commit environment, it must be saved as CJS and not MJS and must use require instead of import.
this works
const fs = require('fs');
const globals = require('globals');
const htmlParser = require('@html-eslint/parser');
const htmlPlugin = require('@html-eslint/eslint-plugin');
const pluginJs = require('@eslint/js');
const pluginReference = require('eslint-plugin-html');
const stylistic = require('@stylistic/eslint-plugin');
module.exports = [
pluginJs.configs.recommended,
stylistic.configs.customize({ jsx: false }),
[...]
this is broken
import fs from 'fs';
import globals from 'globals';
import htmlParser from '@html-eslint/parser';
import htmlPlugin from '@html-eslint/eslint-plugin';
import pluginJs from '@eslint/js';
import pluginReference from 'eslint-plugin-html';
import stylistic from '@stylistic/eslint-plugin';
export default [
pluginJs.configs.recommended,
stylistic.configs.customize({ jsx: false }),
[...]
I will try to restore all previous changes to the workflow and create a new version using the configuration in CSJ.
That's frustratingly specific; excellent work managing to narrow that down!
It is done. Updated and rebased. I hope I managed to rebase the repository correctly, I did it for the first time. 😨
eslint...................................................................Passed
- hook id: eslint
- duration: 1.13s
Preferably merged after https://github.com/godotengine/godot/pull/92013 (jsdoc fix)
🚩UPDATE: jsdoc fix has been merged
Tested locally, and got these two warnings/errors:
Oops! Something went wrong! :(
ESLint: 9.2.0
Error: Cannot find module 'espree'
Require stack:
- C:\Users\Thaddeus\.cache\pre-commit\repos17omlvg\node_env-default\Scripts\node_modules\eslint-plugin-html\src\verifyWithFlatConfigPatch.js
- C:\Users\Thaddeus\.cache\pre-commit\repos17omlvg\node_env-default\Scripts\node_modules\eslint-plugin-html\src\index.js
- D:\Godot\Github\godot\platform\web\eslint.config.cjs
at Module._resolveFilename (node:internal/modules/cjs/loader:1186:15)
at Module._load (node:internal/modules/cjs/loader:1012:27)
at Module.require (node:internal/modules/cjs/loader:1271:19)
at require (node:internal/modules/helpers:123:16)
at verifyExternalHtmlPlugin (C:\Users\Thaddeus\.cache\pre-commit\repos17omlvg\node_env-default\Scripts\node_modules\eslint-plugin-html\src\verifyWithFlatConfigPatch.js:164:17)
at Linter.<anonymous> (C:\Users\Thaddeus\.cache\pre-commit\repos17omlvg\node_env-default\Scripts\node_modules\eslint-plugin-html\src\verifyWithFlatConfigPatch.js:35:35)
at Linter._verifyWithFlatConfigArray (C:\Users\Thaddeus\.cache\pre-commit\repos17omlvg\node_env-default\Scripts\node_modules\eslint\lib\linter\linter.js:2016:21)
at Linter.verify (C:\Users\Thaddeus\.cache\pre-commit\repos17omlvg\node_env-default\Scripts\node_modules\eslint\lib\linter\linter.js:1505:61)
at Linter.verifyAndFix (C:\Users\Thaddeus\.cache\pre-commit\repos17omlvg\node_env-default\Scripts\node_modules\eslint\lib\linter\linter.js:2253:29)
at verifyText (C:\Users\Thaddeus\.cache\pre-commit\repos17omlvg\node_env-default\Scripts\node_modules\eslint\lib\eslint\eslint.js:496:48)
D:\Godot\Github\godot\platform\web\js\libs\library_godot_webgl2.externs.js
0:0 warning File ignored because of a matching ignore pattern. Use "--no-ignore" to disable file ignore settings or use "--no-warn-ignored" to suppress this warning
Both were fixed by tweaking the pre-commit hook to add an --no-warn-ignored arg & espree dependency:
- 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'
Error: Cannot find module 'espree'
Interesting, look like it's a bug in eslint-plugin-html: https://github.com/BenoitZugmeyer/eslint-plugin-html/issues/271
Anyway espree is referred many times in package-lock.json, for example from node_modules/@eslint/eslintrc so it should be installed in node_modules either way.
$ pre-commit run --verbose --all-files eslint
[INFO] Initializing environment for https://github.com/pre-commit/mirrors-clang-format.
[INFO] Initializing environment for https://github.com/psf/black-pre-commit-mirror.
[INFO] Initializing environment for https://github.com/pre-commit/mirrors-mypy.
[INFO] Initializing environment for https://github.com/codespell-project/codespell.
[INFO] Initializing environment for local.
[INFO] Initializing environment for local:@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.
[INFO] Initializing environment for local:jsdoc@^4.0.3.
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
eslint...................................................................Passed
- hook id: eslint
- duration: 2.15s
There are still npm warn ERESOLVE overriding peer dependency warnings from npm when installing node_modules, but they are caused by typescript-eslint and should disappear when version 8 is released. https://github.com/typescript-eslint/typescript-eslint/pull/9002#issuecomment-2106424400
In the meantime, eslint 9.3.0 came out.
https://github.com/eslint/eslint/releases/tag/v9.3.0
In the meantime, eslint 9.3.0 came out.
https://github.com/eslint/eslint/releases/tag/v9.3.0
Might be worth updating then, assuming everything would still work out of the box.
