node: --no-dom-globals CLI flag by indutny · Pull Request #5853 · nodejs/node · GitHub

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

Description of change

Introduce --no-dom-globals CLI flag. With this flag set, following
globals won't be exported:

setTimeout, clearTimeout, setInterval, clearInterval,
setImmediate, clearImmediate
console

These are provided by the DOM implementation in browser, so the
--no-dom-globals flag may be helpful when embedding node.js within
chromium/webkit.

Inspired-By: atom/node@82e10ce

Hey @zcbenz ! I just decided to land some of the commits that you did in atom/node.

May I ask you what was the reasoning behind atom/node@82e10ce#diff-6ff379484cbabad48301d485db111c08R44 ? Would there be any problem for atom, if we won't hide them too?

Also requires flag docs in doc/api/cli.markdown. It's duplication, but each source requires changes that make it impractical to just generate. :)

Spacing?

Not really. There is no \n on previous line.

Maybe --no-browser-globals would be a better naming?

Could you assert.ifError(err) for completeness. And maybe change equal() to strictEqual().

node --break-everything-that-ever-worked-yolo perhaps?

since the target is embedders, perhaps there's a better way of doing this than using a cmdline flag where its usefulness is much more limited?

@rvagg like what?

@Fishrock123 ack cli.markdown

CI: https://ci.nodejs.org/job/node-test-pull-request/2033/

I tend to agree with @rvagg on this... it seems like there ought to be a better way. If this is for embedders, for instance, then perhaps a programmatic configuration hook that allows the builtins to be switched off or replaced? Not sure I see the value of having this as a command line option.

How about we put it in configure? It's not likely to be something you'd want to turn on and off with the same build, you're either building it for this or not. --without-browser-globals -> node.gyp -> NODE_WITHOUT_BROWSER_GLOBALS=1 -> 💥

cc also @rogerwang of nw.js, since I'm sure they also run into this.

Thanks for CCing. It looks good to me either way.

A configure option is a great alternative. Would love to here from @zcbenz tho as to whether that would address the original issue.

Yep, I agree with configure option. Will commit an update in a bit.

@jasnell Pretty sure it would. I was the one who originally suggested a CLI option.

Ok, pushed an update. PTAL

LGTM but I do think it needs to be clear that running in this mode is not officially supported in core (similarly to how we treat building without openssl)

Introduce `--no-browser-globals` configure flag. With this flag set, following globals won't be exported: - `setTimeout`, `clearTimeout`, `setInterval`, `clearInterval`, `setImmediate`, `clearImmediate` - `console` These are provided by the DOM implementation in browser, so the `--no-browser-globals` flag may be helpful when embedding node.js within chromium/webkit. Inspired-By: atom/node@82e10ce

CI: https://ci.nodejs.org/job/node-test-pull-request/2050/

@jasnell thank you, pushed an update! PTAL

@zcbenz PTAL too, it doesn't hide some other features, and I am not sure why you needed to hide them in atom/node . Thanks!

LGTM

@indutny The other globals don't conflict with browser ones. :)

I know, but the original atom commit has more stuff in removed in it.

lgtm but would really like to have @zcbenz weigh in before merging

Hey @zcbenz ! I just decided to land some of the commits that you did in atom/node.

This is awesome, thanks for doing this! And this PR looks good to me.

May I ask you what was the reasoning behind atom/node@82e10ce#diff-6ff379484cbabad48301d485db111c08R44 ? Would there be any problem for atom, if we won't hide them too?

In ancient versions of Node.js those lines used to cause troubles, I think it is good to keep them untouched now.

Great, landing then! @zcbenz thank you! I'm going to port some of the other commits from your repo in one way or another, hope it will simplify node.js update process for electron!

Landed in 8363ede, thank you!

Introduce `--no-browser-globals` configure flag. With this flag set, following globals won't be exported: - `setTimeout`, `clearTimeout`, `setInterval`, `clearInterval`, `setImmediate`, `clearImmediate` - `console` These are provided by the DOM implementation in browser, so the `--no-browser-globals` flag may be helpful when embedding node.js within chromium/webkit. Inspired-By: atom/node@82e10ce PR-URL: #5853 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. (Forrest L Norvell) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344)

Introduce `--no-browser-globals` configure flag. With this flag set, following globals won't be exported: - `setTimeout`, `clearTimeout`, `setInterval`, `clearInterval`, `setImmediate`, `clearImmediate` - `console` These are provided by the DOM implementation in browser, so the `--no-browser-globals` flag may be helpful when embedding node.js within chromium/webkit. Inspired-By: atom/node@82e10ce PR-URL: #5853 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) [npm#6](npm#6) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344)

Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) [npm#6](npm#6) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344) PR-URL: #5970

mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 23, 2016

Fishrock123 reviewed Mar 23, 2016
View reviewed changes

cjihrig reviewed Mar 23, 2016
View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 23, 2016

indutny force-pushed the feature/atom-1 branch from 8b81c56 to 665b83b Compare March 23, 2016 23:52

indutny closed this Mar 26, 2016

indutny deleted the feature/atom-1 branch March 26, 2016 01:04

evanlucas mentioned this pull request Mar 31, 2016

v5.10.0 Release Proposal #5970

Closed

zcbenz mentioned this pull request Apr 3, 2016

Upgrade Node to v5.10.0 electron/electron#4998

Merged

node: --no-dom-globals CLI flag by indutny · Pull Request #5853 · nodejs/node · GitHub

Uh oh!

Uh oh!

node: --no-dom-globals CLI flag - #5853

node: --no-dom-globals CLI flag#5853indutny wants to merge 1 commit intonodejs:masternodejs/node:masterfrom indutny:feature/atom-1indutny/io.js:feature/atom-1Copy head branch name to clipboard

Conversation

indutny commented Mar 23, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

indutny commented Mar 23, 2016

Uh oh!

Fishrock123 commented Mar 23, 2016

Uh oh!

Fishrock123 Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

indutny Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Mar 23, 2016

Uh oh!

cjihrig Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

rvagg commented Mar 23, 2016

Uh oh!

indutny commented Mar 23, 2016

Uh oh!

indutny commented Mar 23, 2016

Uh oh!

indutny commented Mar 23, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

rvagg commented Mar 23, 2016

Uh oh!

Fishrock123 commented Mar 23, 2016

Uh oh!

rogerwang commented Mar 23, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

indutny commented Mar 23, 2016

Uh oh!

Fishrock123 commented Mar 23, 2016

Uh oh!

indutny commented Mar 23, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

indutny commented Mar 23, 2016

Uh oh!

indutny commented Mar 23, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

Fishrock123 commented Mar 24, 2016

Uh oh!

indutny commented Mar 24, 2016

Uh oh!

rvagg commented Mar 24, 2016

Uh oh!

zcbenz commented Mar 26, 2016

Uh oh!

indutny commented Mar 26, 2016

Uh oh!

indutny commented Mar 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

node: --no-dom-globals CLI flag#5853
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:feature/atom-1