[Web] Update to eslint 9 #91771
[Web] Update to eslint 9 #91771
Conversation
|
Note, in flat config mode for eslint 9+ you can create one shared config like this (not finished example): import globals from "globals";
import pluginJs from "@eslint/js";
import pluginReference from "eslint-plugin-html";
export default [
pluginJs.configs.recommended,
// pluginJs.configs.all,
{
rules: {
"indent": ["error", "tab"],
"curly": ["error", "all"],
"quote-props": ["error", "consistent"],
"no-self-assign": "off",
"no-unused-vars": "warn", // FIXME
},
},
{
files: ["js/engine/**/*.js"],
languageOptions: {
globals: {
...globals.browser,
"Features": true,
"Godot": true,
"InternalConfig": true,
"Preloader": true,
},
},
},
{
files: ["js/jsdoc2rst/**/*.js"],
languageOptions: {
globals: globals.node,
},
},
{
files: ["js/libs/**/*.js"],
languageOptions: {
globals: {
...globals.browser,
"autoAddDeps": true,
"Browser": true,
"ERRNO_CODES": true,
"FS": true,
"GL": true,
"GodotConfig": true,
"GodotEventListeners": true,
"GodotFS": true,
"GodotOS": true,
"GodotRuntime": true,
"HEAP32": true,
"HEAP8": true,
"HEAPF32": true,
"HEAPU8": true,
"IDBFS": true,
"IDHandler": true,
"LibraryManager": true,
"mergeInto": true,
"XRWebGLLayer": true,
},
},
},
{
files: ["html/**/*.js"],
languageOptions: {
globals: {
...globals.browser,
"___GODOT_CACHE___": true,
"___GODOT_ENSURE_CROSSORIGIN_ISOLATION_HEADERS___": true,
"___GODOT_OPT_CACHE___": true,
"onClientMessage": true,
},
},
},
{
files: ["html/**/*.html"],
plugins: {
"eslint-plugin-html": pluginReference,
},
languageOptions: {
globals: {
...globals.browser,
"Engine": true,
"$GODOT_CONFIG": true,
"$GODOT_PROJECT_NAME": true,
"$GODOT_THREADS_ENABLED": true,
"___GODOT_THREADS_ENABLED___": true,
},
},
},
{
ignores: [
"eslint.config.mjs",
"**/*.externs.js",
],
},
]; |
|
@patwork amazing, thanks a lot 💙 ! I think |
Faless
force-pushed
the
web/eslint9_prerequisite
branch
from
8f5ec0c to
2864086
Compare
Faless
changed the title
|
As discussed with @patwork this needs some more work to match the old (stricter) rules. I'm leaving this PR in draft as a base for future work. |
| "lint:engine": "cd ../../ && eslint \"platform/web/js/engine/*.js\" --no-config-lookup -c platform/web/eslint.config.mjs", | ||
| "lint:libs": "cd ../../ && eslint \"platform/web/js/libs/*.js\" --no-config-lookup -c platform/web/eslint.config.mjs", | ||
| "lint:tools": "cd ../../ && eslint \"platform/web/js/jsdoc2rst/**/*.js\" --no-config-lookup -c platform/web/eslint.config.mjs", | ||
| "lint:modules": "cd ../../ && eslint \"modules/**/*.js\" --no-config-lookup -c platform/web/eslint.config.mjs", | ||
| "lint:sw": "cd ../../ && eslint \"misc/dist/html/service-worker.js\" --no-config-lookup -c platform/web/eslint.config.mjs", | ||
| "lint:html": "cd ../../ && eslint \"misc/dist/html/*.html\" --no-config-lookup -c platform/web/eslint.config.mjs", |
There was a problem hiding this comment.
We could probably now unify this as a single command I think:
cd ../../ && eslint \"platform/web/js/engine/*.js\" \"platform/web/js/libs/*.js\" \"platform/web/js/jsdoc2rst/**/*.js\" \"modules/**/*.js\" \"misc/dist/html/service-worker.js\" \"misc/dist/html/*.html\" --no-config-lookup -c platform/web/eslint.config.mjs
There was a problem hiding this comment.
Or even:
cd ../../ && eslint \"platform/web/**/*.js\" \"modules/**/*.js\" \"misc/dist/html/*.js\" \"misc/dist/html/*.html\" --no-config-lookup -c platform/web/eslint.config.mjs
|
OK, so this is WIP eslint 9+ flat config with rules.recommended. Next step will be to adapt it to rules.all: import fs from "fs";
import globals from "globals";
import pluginJs from "@eslint/js";
import htmlPlugin from "@html-eslint/eslint-plugin";
import htmlParser from "@html-eslint/parser";
import pluginReference from "eslint-plugin-html";
// eslint 9+ flat config shenanigans
if (!fs.existsSync("./platform/web/eslint.config.mjs")) {
throw Error("eslint must be run from the Godot project root folder");
}
export default [
pluginJs.configs.recommended, // FIXME this should be pluginJs.configs.all
// common rules for all files
{
rules: {
"indent": ["error", "tab"],
"curly": ["error", "all"],
"quote-props": ["error", "consistent"],
"no-self-assign": "off",
"no-unused-vars": ["error", { "args": "none", "caughtErrors": "none" }],
},
},
// jsdoc2rst (node)
{
files: ["platform/web/js/jsdoc2rst/**/*.js"],
languageOptions: {
globals: globals.node,
},
},
// engine files (browser)
{
files: ["platform/web/js/engine/**/*.js"],
languageOptions: {
globals: {
...globals.browser,
"Features": true,
"Godot": true,
"InternalConfig": true,
"Preloader": true,
},
},
},
// libraries and modules (browser)
{
files: [
"platform/web/js/libs/**/*.js",
"modules/**/*.js"
],
languageOptions: {
globals: {
...globals.browser,
"autoAddDeps": true,
"Browser": true,
"ERRNO_CODES": true,
"FS": true,
"GL": true,
"GodotConfig": true,
"GodotEventListeners": true,
"GodotFS": true,
"GodotOS": true,
"GodotRuntime": true,
"HEAP32": true,
"HEAP8": true,
"HEAPF32": true,
"HEAPU8": true,
"IDBFS": true,
"IDHandler": true,
"LibraryManager": true,
"mergeInto": true,
"XRWebGLLayer": true,
},
},
},
// javascript templates (service workers)
{
files: ["misc/dist/html/**/*.js"],
languageOptions: {
globals: {
...globals.browser,
"___GODOT_CACHE___": true,
"___GODOT_ENSURE_CROSSORIGIN_ISOLATION_HEADERS___": true,
"___GODOT_OPT_CACHE___": true,
"onClientMessage": true, // FIXME WHAT IS THIS?
},
},
},
// html templates
{
files: ["misc/dist/html/**/*.html"],
plugins: {
"@html-eslint": htmlPlugin,
"eslint-plugin-html": pluginReference,
},
languageOptions: {
parser: htmlParser,
globals: {
...globals.browser,
"Engine": true,
"$GODOT_CONFIG": true,
"$GODOT_PROJECT_NAME": true,
"$GODOT_THREADS_ENABLED": true,
"___GODOT_THREADS_ENABLED___": true,
},
},
rules: {
...htmlPlugin.configs.recommended.rules,
"@html-eslint/indent": ["error", "tab"],
"@html-eslint/require-closing-tags": ["error", { "selfClosing": "never" }],
},
},
// ignored files
{
ignores: [
"**/eslint.config.mjs",
"**/.eslintrc*.js",
"**/*.externs.js",
],
},
]; |
"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 ./modules ./misc/dist/html",
"fix": "cd ../.. && eslint --fix --no-config-lookup --config ./platform/web/eslint.config.mjs ./platform/web ./modules ./misc/dist/html",
}, |
|
pluginJs.configs.all is crazy, with its default set of rules eslint finds 4400+ problems: |
I actually think that's a bug (left-over dead code using undefined symbols).
Didn't we want to try to stay consistent with the rules? In that case most eslint-disable exceptions should be kept I think.
Again, not sure where specifically, but some catch may be totally fine to fail, especially in engine code where we don't want to spam
Looking great! |
|
The thing is that config.all is more picky than the previous airbnb-base. After restoring all the old rules we're left with:
It's worth considering which ones are worth leaving enabled or can be disabled directly in the javascript code. Also found two unnecessary eslint-disable:
|
|
I think the first 13 rules that are repeated many times in the sources can be safely excluded. The others can be considered, as some of them may actually make it easier to avoid potential errors. Rule no-eq-nullRule eqeqeqRule init-declarationsRule require-unicode-regexpRule prefer-named-capture-groupRule no-useless-assignmentRule consistent-thisRule new-capRule prefer-promise-reject-errorsRule no-warning-commentsRule no-empty-functionRule func-name-matchingRule complexity |
|
After further inspection: Rule no-eq-null❓ Thoughts? Is it too strict? Rule eqeqeq❓ Thoughts? Is it too strict? Rule init-declarationsRule require-unicode-regexp❌ Off. Rule prefer-named-capture-group❌ Off. Rule no-useless-assignmentRule consistent-this❓ Thoughts? Looks like it forces the use of the variable "that" when assigning "this".
Rule new-cap❌ Off. Conflicts with "Godot", "UTF8ToString", etc. Rule prefer-promise-reject-errors❓ Thoughts? godot/platform/web/js/libs/library_godot_input.js Lines 304 to 332 in 2ba22d1 Rule no-warning-commentsRule no-empty-function❌ Off. Rule func-name-matching❓ Thoughts? godot/misc/dist/html/editor.html Line 676 in 2ba22d1 I don't think this function needs a name at all? Rule complexity❌ Off. |
Draft since it needs more work to match the old (stricter) rules.