nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
104.27k stars 28.06k forks source link

tools: migrate to ESLint flat config and update ESLint to v9.2.0 #52780

Open targos opened 2 weeks ago

targos commented 2 weeks ago

Closes: https://github.com/nodejs/node/issues/52567

Not completely ready. I have to double check some things because there are reported errors.

I'm opening the PR to ask a question: For now, I put the entire config in one file. I would like to split it into multiple files to make it more readable but I'm not sure what approach to take. My current idea is to create a new folder, probably somewhere in tools.

nodejs-github-bot commented 2 weeks ago

Review requested:

targos commented 2 weeks ago

@nodejs/linting

aduh95 commented 2 weeks ago

For now, I put the entire config in one file. I would like to split it into multiple files to make it more readable but I'm not sure what approach to take. My current idea is to create a new folder, probably somewhere in tools.

To me, the most obvious choice would be to have them in each directory (e.g. lib/eslint.config.mjs, test/eslint.config.mjs)

targos commented 2 weeks ago

For now, I put the entire config in one file. I would like to split it into multiple files to make it more readable but I'm not sure what approach to take. My current idea is to create a new folder, probably somewhere in tools.

To me, the most obvious choice would be to have them in each directory (e.g. lib/eslint.config.mjs, test/eslint.config.mjs)

I think this would be confusing, because they would not be ESLint config files, just partials for the real config, and it would make people think they work like the old config system.

aduh95 commented 2 weeks ago

Maybe we can use an non-confusing name, lib/eslint.partialconfig.mjs or whatnot.

targos commented 2 weeks ago

SGTM. I'll wait a few days to see if there are other suggestions.

targos commented 1 week ago

Done! I went with eslint.config_partial.mjs so I could colocate it with eslint.config_utils.mjs in tools.

targos commented 1 week ago

Looks like lib/eslint.config_partial.mjs is picked up by js2c.cc. Not sure what's the best way to ignore it. /cc @joyeecheung

anonrig commented 1 week ago

@targos Can we also enable indentation rule for "Makefile" file?

joyeecheung commented 1 week ago

You can hard-code to exclude it from the JS2C like this:

diff --git a/tools/js2c.cc b/tools/js2c.cc
index e0f3d88447..a536b5dcd8 100644
--- a/tools/js2c.cc
+++ b/tools/js2c.cc
@@ -928,6 +928,13 @@ int Main(int argc, char* argv[]) {
   auto mjs_it = file_map.find(".mjs");
   assert(js_it != file_map.end() && mjs_it != file_map.end());

+  auto it = std::find(mjs_it->second.begin(),
+                      mjs_it->second.end(),
+                      "lib/eslint.config_partial.mjs");
+  if (it != mjs_it->second.end()) {
+    mjs_it->second.erase(it);
+  }
+
   std::sort(js_it->second.begin(), js_it->second.end());
   std::sort(mjs_it->second.begin(), mjs_it->second.end());

(Or make it more flexible as another vector to ignore, or make it an argument, or prefix it with a dot and ignore all files beginning with a dot in SearchFiles, if you want)

joyeecheung commented 1 week ago

If you want to ignore all files beginning with a dot, probably just do this (not sure if we have any files starting with a dot that we actually want to include in the binary, I am guessing we don't):

diff --git a/tools/js2c.cc b/tools/js2c.cc
index e0f3d88447..d94fd33a0d 100644
--- a/tools/js2c.cc
+++ b/tools/js2c.cc
@@ -123,6 +123,10 @@ bool SearchFiles(const std::string& dir,
         break;
       }

+      if (StartsWith(dent.name, ".")) {
+        continue;
+      }
+
       std::string path = dir + '/' + dent.name;
       if (EndsWith(path, extension)) {
         files.emplace_back(path);
targos commented 1 week ago

@targos Can we also enable indentation rule for "Makefile" file?

@anonrig Maybe, but it doesn't seem related to what I'm doing here?

targos commented 1 week ago

This is ready for reviews.

nodejs-github-bot commented 1 week ago

CI: https://ci.nodejs.org/job/node-test-pull-request/59027/

targos commented 1 week ago
09:25:44 Running JS linter...
09:25:46 
09:25:46 Oops! Something went wrong! :(
09:25:46 
09:25:46 ESLint: 8.57.0
09:25:46 
09:25:46 Error: EMFILE: too many open files, open '/home/iojs/build/workspace/node-test-linter/test/parallel/test-timers-immediate-unref-nested-once.js'

🤔

MoLow commented 1 week ago

Also worth running on a windows machine before landing

targos commented 1 week ago

I developed it on Windows.

targos commented 1 week ago

Note that .\vcbuild.bat lint-js is already broken on Windows on the main branch because of this file: https://github.com/nodejs/node/blob/main/tools/node_modules/eslint/node_modules/eslint

targos commented 1 week ago

Proof it works after deleting the symlink:

PS D:\Git\nodejs\node> .\vcbuild.bat lint-js nobuild noprojgen
Looking for Python
Python found in C:\Users\Targos\AppData\Local\Microsoft\WindowsApps\\python.exe
Python 3.12.3
Looking for NASM
Jonction créée pour Release <<===>> out\Release
running lint-js
PS D:\Git\nodejs\node>
targos commented 1 week ago

Retrying node-test-linter: https://ci.nodejs.org/job/node-test-linter/54555/

targos commented 1 week ago

It's probably https://github.com/eslint/eslint/issues/18301, only fixed in ESLint v9. I'll add the ESLint update to this PR.

targos commented 1 week ago

Blocked on https://github.com/nodejs/node/pull/52889

targos commented 1 week ago

Added the update to ESLint v9

MoLow commented 1 week ago

Triggered https://ci.nodejs.org/job/node-test-linter/54577/console and it failed :(

20:19:04 Running JS linter...
20:19:04 
20:19:04 Oops! Something went wrong! :(
20:19:04 
20:19:04 ESLint: 9.2.0
20:19:04 
20:19:04 ConfigError: Config (unnamed): Key "rules": Key "constructor-super": structuredClone is not defined
20:19:04     at rethrowConfigError (/home/iojs/build/workspace/node-test-linter/tools/node_modules/eslint/node_modules/@humanwhocodes/config-array/api.js:230:8)
20:19:04     at /home/iojs/build/workspace/node-test-linter/tools/node_modules/eslint/node_modules/@humanwhocodes/config-array/api.js:1032:5
20:19:04     at Array.reduce (<anonymous>)
20:19:04     at FlatConfigArray.getConfig (/home/iojs/build/workspace/node-test-linter/tools/node_modules/eslint/node_modules/@humanwhocodes/config-array/api.js:1028:39)
20:19:04     at FlatConfigArray.isFileIgnored (/home/iojs/build/workspace/node-test-linter/tools/node_modules/eslint/node_modules/@humanwhocodes/config-array/api.js:1060:15)
20:19:04     at /home/iojs/build/workspace/node-test-linter/tools/node_modules/eslint/lib/eslint/eslint-helpers.js:548:38
20:19:04     at Array.forEach (<anonymous>)
20:19:04     at findFiles (/home/iojs/build/workspace/node-test-linter/tools/node_modules/eslint/lib/eslint/eslint-helpers.js:537:11)
20:19:04     at async ESLint.lintFiles (/home/iojs/build/workspace/node-test-linter/tools/node_modules/eslint/lib/eslint/eslint.js:847:27)
20:19:04     at async Object.execute (/home/iojs/build/workspace/node-test-linter/tools/node_modules/eslint/lib/cli.js:500:23)
targos commented 1 week ago

It must be running a very old version of node !

targos commented 1 week ago
19:19:01 + node --version
19:19:01 v16.20.2
richardlau commented 1 week ago
19:19:01 + node --version
19:19:01 v16.20.2

FYI https://github.com/nodejs/build/blob/102f630f31a33cf9762e98bfd10da416d472010c/ansible/roles/jenkins-workspace/tasks/main.yml#L131-L134

targos commented 1 week ago

We need to upgrade the host. Node.js 18+ can't run on Ubuntu 18.

Opened https://github.com/nodejs/build/issues/3713

targos commented 6 days ago

Rebased, reworded the first commit so the PR can be squashed, and separated additional fixes from the ESLint update to ease reviews.

(this is still blocked on updates of the build machines)