| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Review requested:
|
Sorry, something went wrong.
|
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.
|
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.
$ 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. |
Sorry, something went wrong.
This flag was introduced as --experimental-fetch, to enable the API, so we can get feedback without breaking the ecosystem.
Unless I'm missing something, we would still need to add new flags unless we have a --emable-globals flag. |
Sorry, something went wrong.
|
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.
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.
|
node --print crypto doesn't print globalThis.crypto. It prints the module returned by require('crypto') for backwards compatibility. |
Sorry, something went wrong.
|
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).
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). |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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.
Sure. That would solve the “but removing fetch removes four other globals too” issue. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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.
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.
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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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