Skip to content

tools: add lint rule for [await] using in core#64231

Open
Renegade334 wants to merge 2 commits into
nodejs:mainfrom
Renegade334:lint-core-erm
Open

tools: add lint rule for [await] using in core#64231
Renegade334 wants to merge 2 commits into
nodejs:mainfrom
Renegade334:lint-core-erm

Conversation

@Renegade334

Copy link
Copy Markdown
Member

Refs: #64230

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 1, 2026
@Renegade334 Renegade334 added the tools Issues and PRs related to the tools directory. label Jul 1, 2026
Signed-off-by: Renegade334 <contact.9a5d6388@renegade334.me.uk>
Comment thread lib/eslint.config_partial.mjs Outdated
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95

aduh95 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Looks like we were already using using

jasnell
jasnell previously requested changes Jul 4, 2026

@jasnell jasnell 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.

I'm -1 on this for now. Given that ERM is stage 4, I'd rather we just block using the flag that switches it off.

@Renegade334

Copy link
Copy Markdown
Member Author

Given that ERM is stage 4, I'd rather we just block using the flag that switches it off.

My thoughts would be:

  • using usage has a fairly low-effort alternative implementation (try...finally), with the only real caveat being regarding error handling from the disposer, which we explicitly discourage for our own API's disposers anyway.
  • There is no obvious clamour for adding more using cases to core AFAIA. diagnostics_channel is the one place we've seen it, and it's easily refactored away as per the parallel PR.
  • Feature flags are related to stability in Chromium, not ES proposal stage. There are always at least several stage 4 proposals in the V8 flag list; there are currently no fewer than seven in V8 14.6. Is the proposal that we shift to a policy whereby any stage 4 language feature at the point of a V8 major upgrade is forcibly enabled? (Should we disable --v8-disable-temporal-support builds so as that we can rely on Temporal features in core?)
  • Just "switching off" the CLI flag is not a simple task, since all feature flags have IMPLIES relationships, so it would be overridden by eg. --no-js-shipping. We would either need to
    1. block the disabling of any and all js_shipping features, or
    2. float a V8 patch to remove the flag definition, and replace it with our own custom always-on definition uncoupled from js_shipping.

@aduh95

aduh95 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

I agree that the feature being stage 4 is irrelevant here, using using in core makes node crash, that’s not acceptable and adding the lint rule is a good first step. Removing the V8 flag would also be a valid way forward, but it’s unreasonable to expect it to happen in a timely manner.
FWIW my previous attempts at removing a V8 flag, it proved challenging to get someone to review the CL, and it ended up being reverted anyway because it broke something seemingly unrelated (and it was not even syntax related, I have to assume it would be even trickier in this case)

@jasnell please reconsider

@jasnell

jasnell commented Jul 6, 2026

Copy link
Copy Markdown
Member

... using using in core makes node crash

Only when a non-default v8 option is used. From our own docs, "V8's options have no stability guarantee. The V8 team themselves don't consider them to be part of their formal API, and reserve the right to change them at any time. Likewise, they are not covered by the Node.js stability guarantees."

I'll clear my block but I still fundamentally disagree.

@jasnell jasnell dismissed their stale review July 6, 2026 16:20

While I still disagree with the change, I won't block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants