GeezerVison is showing github.com — cli: add flag for disabling globals by GeoffreyBooth · Pull Request #50554 · nodejs/node
Conversation
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
|
Review requested:
- @nodejs/startup
- @nodejs/web-standards
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
aduh95
commented
Nov 4, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
If i specify a global that’s not permitted to be suppressed, does node fail to start? I’d hope so.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
-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?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
aduh95
commented
Nov 5, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
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.
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.
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.
|