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: revert to use @stylistic/eslint-plugin-js v3 #57314

Merged
merged 2 commits into from
Mar 4, 2025

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 4, 2025

@stylistic/eslint-plugin-js v4 has been updated to ship ESM-only, which needs require(esm) support due to the monkey-patching we do in our eslint.config.mjs. The node-test-linter job in Jenkins still runs Node.js v20 which has not yet released unflagging of require(esm). Revert to use v3 for now until we upgrade the Node.js version used in the CI.

Refs: #57261
For an example of broken CI, see https://ci.nodejs.org/job/node-test-linter/59234/console

@stylistic/eslint-plugin-js v4 has been updated to ship ESM-only,
which needs require(esm) support due to the monkey-patching
we do in our eslint.config.mjs. The node-test-linter job in Jenkins
still runs Node.js v20 which has not yet released unflagging
of require(esm). Revert to use v3 for now until we upgrade the
Node.js version used in the CI.
@joyeecheung joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 4, 2025
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Mar 4, 2025
Copy link
Contributor

github-actions bot commented Mar 4, 2025

Fast-track has been requested by @joyeecheung. Please 👍 to approve.

@joyeecheung
Copy link
Member Author

Arguably a better fix would be to make this less hacky

node/eslint.config.mjs

Lines 33 to 38 in a7f648c

Module._resolveFilename = (request, parent, isMain, options) => {
if (hacks.includes(request) && parent.id.endsWith('__placeholder__.js')) {
return resolveEslintTool(request);
}
return ModuleResolveFilename(request, parent, isMain, options);
};

Or to upgrade the CI to use Node.js v22. But the issue is now breaking the CI and this seems like the most no-brainer fix.

@aduh95
Copy link
Contributor

aduh95 commented Mar 4, 2025

You need to update the package-lock.json

@richardlau
Copy link
Member

Ideally PR's such as #57261 would have a Jenkins CI run.

@targos
Copy link
Member

targos commented Mar 4, 2025

What do you think about changing the workflow so it also runs with Node.js 20.x so we are more confident about the fix and less likely to have this kind of issues in the future?

@joyeecheung
Copy link
Member Author

What do you think about changing the workflow so it also runs with Node.js 20.x so we are more confident about the fix and less likely to have this kind of issues in the future?

I am not sure how the Jenkins gets the Node.js versions, but if it's an easy fix, happy that someone else get it done ASAP instead.

@richardlau
Copy link
Member

What do you think about changing the workflow so it also runs with Node.js 20.x so we are more confident about the fix and less likely to have this kind of issues in the future?

I am not sure how the Jenkins gets the Node.js versions, but if it's an easy fix, happy that someone else get it done ASAP instead.

On Jenkins the jenkins-workspace machines (which run the linter job among others) are setup via Ansible: nodejs/build#4032 (comment)

targos added a commit to targos/node that referenced this pull request Mar 4, 2025
@targos
Copy link
Member

targos commented Mar 4, 2025

Alternative that removes the use of require: #57315

@joyeecheung
Copy link
Member Author

node-test-linter CI: https://ci.nodejs.org/job/node-test-linter/59242/

targos added a commit to targos/node that referenced this pull request Mar 4, 2025
@jasnell
Copy link
Member

jasnell commented Mar 4, 2025

I'm good with either approach. thanks for taking a look

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 4, 2025
@joyeecheung
Copy link
Member Author

Landing it now to unbreak CI. We can revert it when the Node.js version gets updated or when #57315 lands

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 4, 2025
@nodejs-github-bot nodejs-github-bot merged commit e05756f into nodejs:main Mar 4, 2025
26 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in e05756f

nodejs-github-bot pushed a commit that referenced this pull request Mar 7, 2025
Refs: #57314
PR-URL: #57315
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fast-track PRs that do not need to wait for 48 hours to land. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants