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

cli: add flag for disabling globals #50554

Closed
wants to merge 2 commits into from

Conversation

GeoffreyBooth
Copy link
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

@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 APIs labels Nov 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@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 related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Nov 4, 2023
@GeoffreyBooth
Copy link
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.

@GeoffreyBooth
Copy link
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.

@aduh95
Copy link
Contributor

aduh95 commented Nov 4, 2023

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.

@aduh95
Copy link
Contributor

aduh95 commented Nov 4, 2023

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.

@ljharb
Copy link
Member

ljharb commented Nov 5, 2023

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

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Nov 5, 2023

-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?

@targos
Copy link
Member

targos commented Nov 5, 2023

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

@aduh95
Copy link
Contributor

aduh95 commented Nov 5, 2023

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.

@bnoordhuis
Copy link
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.

@GeoffreyBooth
Copy link
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.

@aduh95
Copy link
Contributor

aduh95 commented Nov 5, 2023

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.

@GeoffreyBooth
Copy link
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.

@aduh95
Copy link
Contributor

aduh95 commented Nov 5, 2023

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.

@GeoffreyBooth
Copy link
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.

@GeoffreyBooth GeoffreyBooth deleted the disable-global-flag branch November 8, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 related to general changes in the lib or src directory. 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 APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants