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

lib: require globals instead of using the global proxy #27112

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
10 participants
@joyeecheung
Copy link
Member

commented Apr 6, 2019

In addition, use process.stderr instead of console.error when
there is no need to swallow the error.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@targos

targos approved these changes Apr 6, 2019

@devsnek

devsnek approved these changes Apr 6, 2019

@nodejs-github-bot

This comment has been minimized.

@ZYSzys

ZYSzys approved these changes Apr 6, 2019

@jasnell

jasnell approved these changes Apr 6, 2019

@nodejs-github-bot

This comment has been minimized.

@BridgeAR
Copy link
Member

left a comment

What's the benefit of using require instead of relying on the global proxy? Using the latter seems just simpler in this case?

Show resolved Hide resolved lib/internal/process/warning.js

Should have been a comment.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

@joyeecheung I am not sure what the benefit of using require instead of relying on the global proxy is in these cases. Using the latter seems just simpler?

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

I am not sure what the benefit of using require instead of relying on the global proxy is in these cases. Using the latter seems just simpler?

The latter is still subject to monkey-patching, so one could delete global.setTimeout and Node.js would um..blow up. It is the same kind of defensive measure as the primordials - if we are defending JS builtins, I don't see why not to defend the Node.js builtins as well?

lib: require globals instead of using the global proxy
In addition, use process.stderr instead of console.error when
there is no need to swallow the error.

@joyeecheung joyeecheung force-pushed the joyeecheung:remove-globals branch from 3c95f90 to 24644ec Apr 9, 2019

@nodejs-github-bot

This comment has been minimized.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

@BridgeAR Are your comments blocking?

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@joyeecheung

The latter is still subject to monkey-patching, so one could delete global.setTimeout and Node.js would um..blow up. It is the same kind of defensive measure as the primordials - if we are defending JS builtins, I don't see why not to defend the Node.js builtins as well?

The builtins could still be directly manipulated (as in: the properties of the exports could be changed). For me the primordials are different since especially legacy code tends to add extra properties / functions on the prototype or changes behavior of some functions. Deleting properties from the global is something I never encountered so far (but it might still be done). But should we really guard against this case while not being able to guard against monkey patching the builtin in general?

Are your comments blocking?

I would like to at least discuss this a bit further.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

But should we really guard against this case while not being able to guard against monkey patching the builtin in general?

I think we should, or at least we should not start with letting things loose by default. If anyone actually has serious use case for this, we could explicitly use global.something and add a comment, but not the other way around. Leaving the monkey-patching contract loose does not really do good for anybody, IMO.

Specifically, what's the downside of guarding against monkey-patching like this? This is just a +42 −24 change, it's not really complex.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

@BridgeAR Does #27112 (comment) answer your question?

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Ping @BridgeAR

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Landed in a38e9c4

joyeecheung added a commit that referenced this pull request Apr 15, 2019

lib: require globals instead of using the global proxy
In addition, use process.stderr instead of console.error when
there is no need to swallow the error.

PR-URL: #27112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@dsanders11

This comment has been minimized.

Copy link

commented Jun 22, 2019

@BridgeAR, @joyeecheung, apologies for commenting on an old closed PR, but wanted to add a 👍for the change.

Name clashes between globals can cause bugs in Electron's renderer process (like electron/electron#14915) when code in Node accidentally uses the global from window. In particular the issue I referenced is caused by Node using the global version of clearTimeout for a timer it created using it's own timers API, leading to the timeout not being cleared. That issue would be fixed by this PR. Of course there's still the potential for that issue in third-party libraries written for Node, but that's more of an Electron issue.

Anyway, just throwing in my 2 cents on why this is a good PR.

dsanders11 added a commit to dsanders11/node that referenced this pull request Jun 22, 2019

lib: require globals instead of using the global proxy
In addition, use process.stderr instead of console.error when
there is no need to swallow the error.

PR-URL: nodejs/node#27112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MarshallOfSound added a commit to electron/node that referenced this pull request Jun 26, 2019

lib: require globals instead of using the global proxy
In addition, use process.stderr instead of console.error when
there is no need to swallow the error.

PR-URL: nodejs/node#27112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.