GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — node: --no-dom-globals CLI flag by indutny · Pull Request #5853 · nodejs/node

Conversation

@indutny

@indutny indutny commented Mar 23, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@indutny

indutny commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex mscdex added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Mar 23, 2016
@Fishrock123

Fishrock123 commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123
Fishrock123 reviewed Mar 23, 2016
View reviewed changes
Comment thread src/node.cc Outdated

@Fishrock123 Fishrock123 Mar 23, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@indutny indutny Mar 23, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Fishrock123

Fishrock123 commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig reviewed Mar 23, 2016
View reviewed changes
Comment thread test/parallel/test-cli-options.js Outdated

@cjihrig cjihrig Mar 23, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@rvagg

rvagg commented Mar 23, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

@rvagg like what?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

@Fishrock123 ack cli.markdown
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 23, 2016
@rvagg

rvagg commented Mar 23, 2016

Copy link
Copy Markdown
Member

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 -> 💥
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rogerwang

rogerwang commented Mar 23, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

Ok, pushed an update. PTAL
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny
node: --no-browser-globals configure flag
665b83b 
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
@indutny
indutny force-pushed the feature/atom-1 branch from 8b81c56 to 665b83b Compare March 23, 2016 23:52
@indutny

indutny commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

@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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Mar 24, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rvagg

rvagg commented Mar 24, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zcbenz

zcbenz commented Mar 26, 2016

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Mar 26, 2016

Copy link
Copy Markdown
Member Author

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Mar 26, 2016

Copy link
Copy Markdown
Member Author

Landed in 8363ede, thank you!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny indutny closed this Mar 26, 2016
@indutny
indutny deleted the feature/atom-1 branch March 26, 2016 01:04
indutny added a commit that referenced this pull request Mar 26, 2016
@indutny
node: --no-browser-globals configure flag
8363ede 
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 <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
@indutny @evanlucas
node: --no-browser-globals configure flag
2fa959b 
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 <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 31, 2016
v5.10.0 Release Proposal #5970
Closed
evanlucas added a commit that referenced this pull request Mar 31, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
9cd6e98 
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)
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
@indutny @evanlucas
node: --no-browser-globals configure flag
e8cdf9a 
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 <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
evanlucas added a commit that referenced this pull request Mar 31, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
1bb2c4b 
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)
evanlucas added a commit that referenced this pull request Apr 1, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
79ea8c3 
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)
evanlucas added a commit that referenced this pull request Apr 1, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
64c87e2 
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
@zcbenz zcbenz mentioned this pull request Apr 3, 2016
Upgrade Node to v5.10.0 electron/electron#4998
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

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.

8 participants

@indutny @Fishrock123 @rvagg @jasnell @rogerwang @zcbenz @cjihrig @mscdex
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.