src: use the config binding to carry --no-browser-globals - #26228
src: use the config binding to carry --no-browser-globals#26228joyeecheung wants to merge 1 commit into
Conversation
Sorry, something went wrong.
|
@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2680/pipeline |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
This changes the behavior to the former.
Before, this variable was only set in case it was true. Now it's set unconditionally.
It is now also set as a regular property and not only as read-only.
Sorry, something went wrong.
Instead of setting it in the process object, since this is a configure-time option. Also added a shim that can be deprecated and removed some time later.
|
@BridgeAR Thanks for review, updated. CI: https://ci.nodejs.org/job/node-test-pull-request/21255/ ✔️ |
Sorry, something went wrong.
Instead of setting it in the process object, since this is a configure-time option. Also added a shim that can be deprecated and removed some time later. PR-URL: nodejs#26228 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Instead of setting it in the process object, since this is a configure-time option. Also added a shim that can be deprecated and removed some time later. PR-URL: nodejs#26228 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Instead of setting it in the process object, since this is a configure-time option. Also added a shim that can be deprecated and removed some time later. PR-URL: #26228 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
|
This is actually causing some problems in Electron. We load Node in 2 different environments:
By turning this config into compile-time only, we are not able to switch modes at runtime. /cc @nitsakh @codebytere |
Sorry, something went wrong.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
|
@zcbenz Hi, I am pretty sure process.noBrowserGlobals was already processed pretty early during to bootstrap so after that it was not possible to change the browser globals at run time anyway? --no-browser-globals was already a build time flag before this patch, it was not a valid CLI option for node. This patch just moves process.noBrowserGlobals to process.binding('config').noBrowserGlobals and shim it later so that it would be easier to just remove process.noBrowserGlobals some time. I think we can make the installation dynamic at run time, though process.noBrowserGlobals is not an ideal place to do that. For example, we can make process.features.browserGlobals an accessor property and toggle the globals dynamically |
Sorry, something went wrong.
|
@joyeecheung We hijacked the bootstrap script to install some Electron specific things to Node. Having a dynamic flag would be great. |
Sorry, something went wrong.
|
@zcbenz If you just hijack the property passed into bootstrap script and only need it once, maybe the easiest workaround is to hijack process.binding('config') (in src/node_config.cc) instead. |
Sorry, something went wrong.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
This is a temporary hack in responding to Node's change: nodejs/node#26228 We need to figure out a better solution.
Instead of setting it in the process object, since this is
a configure-time option. Also added a shim that can be
deprecated and removed some time later.
Checklist