GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — cli: add flag for disabling globals by GeoffreyBooth · Pull Request #50554 · nodejs/node

Conversation

@GeoffreyBooth

@GeoffreyBooth GeoffreyBooth commented Nov 4, 2023

Copy link
Copy Markdown
Member

Per #50412 (comment), this creates a new flag --disable-global to provide the user a way to disable certain global APIs. The globals that can be disabled include CustomEvent, fetch (and its associated FormData, Headers, Request, and Response), navigator, and WebSocket.

The motivation for this flag is both to provide a compatibility escape hatch and also to abstract this ability for future global APIs. We currently have several flags for specific web APIs, like --no-experimental-fetch and --no-experimental-global-customevent; as we continue to add web APIs, which are often globals, these per-API flags will proliferate. Rather than continue to grow our long list of CLI flags, a single flag that takes multiple options seems preferable.

@nodejs/web-standards @nodejs/tsc

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

styfle and bricss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
GeoffreyBooth added 2 commits November 4, 2023 15:40
@GeoffreyBooth
cli: add flag for disabling globals
3dfe77a
@GeoffreyBooth
implement flag
407ae89
@GeoffreyBooth GeoffreyBooth added cli Issues and PRs related to the Node.js command-line interface. experimental Issues and PRs related to experimental features. web-standards Issues and PRs related to web-platform APIs and standards compliance. labels Nov 4, 2023
@GeoffreyBooth
GeoffreyBooth requested a review from anonrig November 4, 2023 23:09
@nodejs-github-bot

nodejs-github-bot commented Nov 4, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup
  • @nodejs/web-standards
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Nov 4, 2023
@GeoffreyBooth

GeoffreyBooth commented Nov 4, 2023

Copy link
Copy Markdown
Member Author

One concern I had while working on this is that there seems to be a progression where features move from pre_execution.js to exposed-window-or-worker.js when they go stable; is there a benefit to this, perhaps because the latter is in the snapshot and the former is not? Yet fetch hasn’t made this transition, even though it is stable. If it is desired for globals to eventually be declared in exposed-window-or-worker.js, I think maybe we should just mention in the docs under --disable-global that names on the list may disappear in future semver major versions of Node as those APIs reach stability.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth

GeoffreyBooth commented Nov 4, 2023

Copy link
Copy Markdown
Member Author

I was also surprised to learn that --no-experimental-global-webcrypto doesn’t remove globalThis.crypto. See #46079. If this is unintentional, it’s perhaps something to be fixed in a separate PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Nov 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I was also surprised to learn that --no-experimental-global-webcrypto doesn’t remove globalThis.crypto. See #46079. If this is unintentional, it’s perhaps something to be fixed in a separate PR.

$ echo 'console.log("crypto" in globalThis)' | node
true
$ echo 'console.log("crypto" in globalThis)' | node --no-experimental-global-webcrypto
false

--no-experimental-global-webcrypto doesn’t remove globalThis.crypto indeed, it never defines the WebCrypto globals to the global object in the first place. Regardless, that doesn't change much from the user perspective, it's not there at runtime.
I.E. when --no-experimental-global-webcrypto, globalThis.crypto will never be equal to require('node:crypto').webcrypto – unless userland mutated the global object of course.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Nov 4, 2023

Copy link
Copy Markdown
Contributor

We currently have several flags for specific web APIs, like --no-experimental-fetch

This flag was introduced as --experimental-fetch, to enable the API, so we can get feedback without breaking the ecosystem.

as we continue to add web APIs, which are often globals, these per-API flags will proliferate. Rather than continue to grow our long list of CLI flags, a single flag that takes multiple options seems preferable.

Unless I'm missing something, we would still need to add new flags unless we have a --emable-globals flag.

tniessen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ljharb

ljharb commented Nov 5, 2023

Copy link
Copy Markdown
Member

If i specify a global that’s not permitted to be suppressed, does node fail to start? I’d hope so.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth

GeoffreyBooth commented Nov 5, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

-no-experimental-global-webcrypto doesn’t remove globalThis.crypto indeed, it never defines the WebCrypto globals to the global object in the first place.

I was testing via node --no-experimental-global-webcrypto --print crypto and it was printing what at a glance appeared to be the same global as without the flag. Is this expected?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented Nov 5, 2023

Copy link
Copy Markdown
Member

node --print crypto doesn't print globalThis.crypto. It prints the module returned by require('crypto') for backwards compatibility.

tniessen and aduh95 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Nov 5, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

What do we think about --disable-global-api for the name? Since more often than not it disables a bunch of variables and not just the one provided, it would be better if there can be no surprises. (Also, we might get feature request for node --disable-global=fs -p 'globalThis.fs === undefined, and I think we simply don't need the complexity).

node --print crypto doesn't print globalThis.crypto. It prints the module returned by require('crypto') for backwards compatibility.

To add to that, both node --no-experimental-global-webcrypto --print crypto and node --experimental-global-webcrypto --print crypto will output a reference to node:crypto; on the other hand, node --experimental-global-webcrypto --print 'globalThis.crypto' will output a reference to require('node:crypto').webcrypto, while node --no-experimental-global-webcrypto --print 'globalThis.crypto' will output a reference to node:crypto (the same way globalThis.fs is a reference to node:fs in this context).
A lot of thoughts went into this, so the introduction of WebCrypto globals could be as non breaking as possible. I have the feeling it's dragging us off-topic.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Nov 5, 2023

Copy link
Copy Markdown
Member

I don't see the point of this flag. It creates a non-standard runtime that makes libraries more likely to break. No library author is going to test the complete matrix of globals that can be erased.

It's also not like removing globals at startup isn't something you can't already do. This flag has zero merit, IMO.

tniessen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95 aduh95 mentioned this pull request Nov 5, 2023
lib: add --no-experimental-global-navigator CLI flag #50562
Merged
@GeoffreyBooth

GeoffreyBooth commented Nov 5, 2023

Copy link
Copy Markdown
Member Author

Unless I’m missing something, we would still need to add new flags unless we have a --enable-globals flag.

For globals that we add flagged, like behind flags like --experimental-websocket, yes. But I was assuming there would be other globals potentially added unflagged like navigator. Forgive me for potentially sounding controversial, but I don’t think it was a mistake to release navigator unflagged. Having an --experimental-navigator flag wouldn’t have prevented the bug report that came in, because that user was surprised by code that suddenly broke that had worked before; that user wasn’t trying to use navigator, and therefore wouldn’t have used an --experimental-navigator flag. What would have helped that user would have been a --no-experimental-navigator flag, which I suppose we get automatically with an --experimental-navigator flag, but now every new global needs a one-off flag created for it. The idea behind --disable-globals is to abstract that work, so that it’s just one here and any future global API that isn’t controversial enough to need its own flag can rely on --disable-global as the compatibility escape hatch.

The other motivation was to limit the endless growth of flags. I find it terrible for UX that we have over a hundred flags. If these “disable” flags are temporary, like if we plan to remove --no-experimental-fetch now that it’s stable, then I’m a lot less bothered by --no-experimental-global-navigator. I think this would actually be preferable, assuming I’m correct in my assumption above that anything that a flag can disable can’t be included in the snapshot (?) unless we change the effect of the flag from preventing adding the global to instead deleting it.

For me that’s the big question: is disabling certain globals meant to be temporary, until the new global is stable, or is it a permanent feature we want to support? If we need a way to disable navigator in perpetuity, I’d prefer we have --disable-globals so that we don’t add a new permanent flag just for disabling navigator. If the ability to disable navigator (and whatever similar APIs come after it) is meant to be temporary, then I would prefer #50562 so that we have flexibility in where we define these globals in order to put them in or out of the snapshot.

What do we think about --disable-global-api for the name?

Sure. That would solve the “but removing fetch removes four other globals too” issue.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Nov 5, 2023

Copy link
Copy Markdown
Contributor

Landing new globals behind a flag first has an upside you forgot to mention: it makes their introduction non-semver-major, allowing backports and therefore earlier feedback plus it makes it easier for library maintainers to reproduce/isolate bugs related to the new globals.

I think those experimental flags should go (become no op) at some point, maybe once all the non-EOL release lines have the global by default.

ljharb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth

GeoffreyBooth commented Nov 5, 2023

Copy link
Copy Markdown
Member Author

I think those experimental flags should go (become no op) at some point

They can't become no-ops because then they'd be broken; like --no-experimental-fetch really should disable fetch, I'd think.

If they're all meant to be temporary then sure, I prefer #50562.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Nov 5, 2023

Copy link
Copy Markdown
Contributor

They can't become no-ops because then they'd be broken; like --no-experimental-fetch really should disable fetch, I'd think.

When a boolean flag becomes a no-op, it can stay as only one form – e.g. you can still do node --experimental-top-level-await …, but node --no-experimental-top-level-await … throws.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth

GeoffreyBooth commented Nov 6, 2023

Copy link
Copy Markdown
Member Author

When a boolean flag becomes a no-op, it can stay as only one form – e.g. you can still do node --experimental-top-level-await …, but node --no-experimental-top-level-await … throws.

I was thinking of --experimental-top-level-await and --no-experimental-top-level-await as two separate flags. But sure, it sounds like we’re in agreement. So whenever enough time has passed, we will make --experimental-fetch a no-op and --no-experimental-fetch will throw? And likewise for navigator when its time comes?

If that’s the case then yes I prefer #50562, I’d rather not support disabling globals indefinitely if they can graduate to being permanently enabled. This PR was made under the assumption that the ability to disable would need to be provided indefinitely.

styfle reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth GeoffreyBooth closed this Nov 8, 2023
@GeoffreyBooth
GeoffreyBooth deleted the disable-global-flag branch November 8, 2023 18:22
@daneryl daneryl mentioned this pull request Aug 27, 2026
Tech debt: remove --no-experimental-fetch flag huridocs/uwazi#9632
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig Awaiting requested review from anonrig

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command-line interface. experimental Issues and PRs related to experimental features. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. web-standards Issues and PRs related to web-platform APIs and standards compliance.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@GeoffreyBooth @nodejs-github-bot @aduh95 @ljharb @targos @bnoordhuis
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.