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

joyeecheung
Copy link
Member

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
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 6, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

BridgeAR
BridgeAR previously approved these changes Apr 7, 2019
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

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

lib/internal/process/warning.js Show resolved Hide resolved
@BridgeAR BridgeAR dismissed their stale review April 7, 2019 12:40

Should have been a comment.

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member

BridgeAR 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
Copy link
Member Author

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?

In addition, use process.stderr instead of console.error when
there is no need to swallow the error.
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

@BridgeAR Are your comments blocking?

@BridgeAR
Copy link
Member

@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
Copy link
Member Author

joyeecheung 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
Copy link
Member Author

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

@joyeecheung
Copy link
Member Author

Ping @BridgeAR

@joyeecheung
Copy link
Member Author

Landed in a38e9c4

joyeecheung added a commit that referenced this pull request Apr 15, 2019
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@dsanders11
Copy link
Contributor

@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.

MarshallOfSound pushed a commit to electron/node that referenced this pull request Jun 26, 2019
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 <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants