Skip to content
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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

targos
Copy link
Member

@targos targos commented May 1, 2024

Closes: #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.

@targos targos added the wip Issues and PRs that are still a work in progress. label May 1, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/web-standards

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 1, 2024
@targos
Copy link
Member Author

targos commented May 2, 2024

@nodejs/linting

eslint.config.mjs Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented May 2, 2024

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
Copy link
Member Author

targos commented May 2, 2024

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
Copy link
Contributor

aduh95 commented May 2, 2024

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

@targos
Copy link
Member Author

targos commented May 2, 2024

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

@targos targos removed the wip Issues and PRs that are still a work in progress. label May 6, 2024
@targos
Copy link
Member Author

targos commented May 6, 2024

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

@targos
Copy link
Member Author

targos commented May 6, 2024

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
Copy link
Member

anonrig commented May 6, 2024

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

@joyeecheung
Copy link
Member

joyeecheung commented May 7, 2024

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
Copy link
Member

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
Copy link
Member Author

targos commented May 7, 2024

@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 targos added the review wanted PRs that need reviews. label May 8, 2024
@targos
Copy link
Member Author

targos commented May 8, 2024

This is ready for reviews.

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label May 8, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 8, 2024
@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member Author

targos commented May 8, 2024

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
Copy link
Member

MoLow commented May 8, 2024

Also worth running on a windows machine before landing

@targos
Copy link
Member Author

targos commented May 8, 2024

I developed it on Windows.

@targos
Copy link
Member Author

targos commented May 8, 2024

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
Copy link
Member Author

targos commented May 8, 2024

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
Copy link
Member Author

targos commented May 8, 2024

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

@targos
Copy link
Member Author

targos commented May 8, 2024

It's probably eslint/eslint#18301, only fixed in ESLint v9. I'll add the ESLint update to this PR.

@targos targos marked this pull request as draft May 8, 2024 11:07
@targos
Copy link
Member Author

targos commented May 8, 2024

Blocked on #52889

@targos targos changed the title tools: migrate to ESLint flat config tools: migrate to ESLint flat config and update ESLint to v9.2.0 May 9, 2024
@targos targos marked this pull request as ready for review May 9, 2024 17:05
@targos
Copy link
Member Author

targos commented May 9, 2024

Added the update to ESLint v9

@MoLow
Copy link
Member

MoLow commented May 9, 2024

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
Copy link
Member Author

targos commented May 9, 2024

It must be running a very old version of node !

@targos
Copy link
Member Author

targos commented May 9, 2024

19:19:01 + node --version
19:19:01 v16.20.2

@richardlau
Copy link
Member

@targos
Copy link
Member Author

targos commented May 10, 2024

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

Opened nodejs/build#3713

@targos targos added the blocked PRs that are blocked by other issues or PRs. label May 10, 2024
@targos targos added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label May 10, 2024
@targos
Copy link
Member Author

targos commented May 10, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate ESLint config to flat config
7 participants