Skip to content

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

Merged
nodejs-github-bot merged 6 commits into
nodejs:mainfrom
targos:eslint-flat-config
May 23, 2024
Merged

tools: migrate to ESLint flat config and update ESLint to v9.3.0#52780
nodejs-github-bot merged 6 commits into
nodejs:mainfrom
targos:eslint-flat-config

Conversation

@targos

Copy link
Copy Markdown
Member

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.

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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/web-standards

@nodejs-github-botnodejs-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
Comment threadeslint.config.mjs Outdated
@targos

Copy link
Copy Markdown
MemberAuthor

@nodejs/linting

Comment threadeslint.config.mjs Outdated
@aduh95

Copy link
Copy Markdown
Contributor

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

targos commented May 2, 2024

Copy link
Copy Markdown
MemberAuthor

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

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

@targos

Copy link
Copy Markdown
MemberAuthor

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

@targos
targosforce-pushed the eslint-flat-config branch from 7092731 to e8d08e1CompareMay 6, 2024 10:01
@targostargos removed the wip Issues and PRs that are still a work in progress. label May 6, 2024
@targos

Copy link
Copy Markdown
MemberAuthor

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

@targos

Copy link
Copy Markdown
MemberAuthor

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

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

@joyeecheung

joyeecheung commented May 7, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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

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

@targostargos added the review wanted PRs that need reviews. label May 8, 2024
@targos

Copy link
Copy Markdown
MemberAuthor

This is ready for reviews.

@MoLowMoLow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@targostargos added the request-ci Add this label to start a Jenkins CI on a PR. label May 8, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 8, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@targos

Copy link
Copy Markdown
MemberAuthor
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

MoLow commented May 8, 2024

Copy link
Copy Markdown
Member

Also worth running on a windows machine before landing

@targos

Copy link
Copy Markdown
MemberAuthor

I developed it on Windows.

@targos

Copy link
Copy Markdown
MemberAuthor

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

targos commented May 8, 2024

Copy link
Copy Markdown
MemberAuthor

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
targos marked this pull request as draft May 22, 2024 13:06
@targos
targos marked this pull request as ready for review May 22, 2024 15:31
@targostargos added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. request-ci Add this label to start a Jenkins CI on a PR. labels May 22, 2024
@targos
targosforce-pushed the eslint-flat-config branch from dc6f540 to 1739756CompareMay 22, 2024 15:33
@targostargos added the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@targos

Copy link
Copy Markdown
MemberAuthor

CleanShot 2024-05-22 at 17 46 08@2x

┬─┬ノ( º _ ºノ)

@anonriganonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

gogogogogogogogogogo

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@targos

Copy link
Copy Markdown
MemberAuthor

@aduh95 Would you like to have another look?

Comment on lines +28 to +35
export const noRestrictedSyntaxCommonTest = [
{
// TODO(@panva): move this to no-restricted-properties
// when https://github.com/eslint/eslint/issues/16412 is fixed.
selector: "Identifier[name='webcrypto']",
message: 'Use `globalThis.crypto`.',
},
];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might make sense to wait for #53023 to land and get rid of this.

@anonriganonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label May 23, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 23, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 7e6d92c into nodejs:mainMay 23, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 7e6d92c

@targos
targos deleted the eslint-flat-config branch May 26, 2024 09:03
targos added a commit that referenced this pull request Jun 1, 2024
Closes: #52567
PR-URL: #52780Fixes: #52567
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Jun 7, 2024
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
Closes: nodejs#52567
PR-URL: nodejs#52780Fixes: nodejs#52567
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.review wantedPRs that need reviews.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate ESLint config to flat config

8 participants

@targos@nodejs-github-bot@aduh95@anonrig@joyeecheung@MoLow@richardlau@marco-ippolito