lib: require globals instead of using the global proxy

[joyeecheung]

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]

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3186

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[BridgeAR]

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

Should have been a comment.

[nodejs-github-bot]

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

[BridgeAR]

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

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?

[nodejs-github-bot]

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

[joyeecheung]

@BridgeAR Are your comments blocking?

[BridgeAR]

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

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]

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

[joyeecheung]

Ping @BridgeAR

[joyeecheung]

Landed in a38e9c4

[dsanders11]

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