-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
util,console: guard against overwritten util functions #13011
Conversation
Some functions in util and console will happily invoke other functions in `util` that have been overridden. The internal use of these functions is an implementation detail and users should not be able to indirectly affect them this way.
I don’t think that’s actually true… If I was overriding |
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.
1 Question
1 Licese comment
@@ -0,0 +1,92 @@ | |||
// Copyright Joyent, Inc. and other Node contributors. |
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.
should it be
Copyright Node.js contributors. All rights reserved.
SPDX-License-Identifier: MIT
As per #10599 (comment)
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.
Since most of the test code here is copied nearly verbatim from another test, I think keeping that test's license is appropriate. I added a few lines, but that's it. I made it a separate test to avoid having to wrestle with side effects from this test affecting other tests.
@@ -103,7 +103,7 @@ function write(ignoreErrors, stream, string, errorhandler) { | |||
Console.prototype.log = function log(...args) { | |||
write(this._ignoreErrors, | |||
this._stdout, | |||
`${util.format.apply(null, args)}\n`, | |||
`${format.apply(null, args)}\n`, |
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.
[question] if args
is a restArgs, why not just spread them?
RE CI: ignore fails on macOS & freeBSD, we're fixing that (#12964) I'm -1 on this. IMHO too deep down the slippery slope... (#12960 (comment)) |
@addaleax The docs agree with you. They state that I'm going to close this, but if anyone thinks it's worth further consideration, please comment and I'll reopen. /cc @daurnimator just in case this is relevant to somewhat similar issues they've raised elsewhere |
@Trott no this change is unrelated to the issues I've raised. |
Some functions in util and console will happily invoke other functions
in
util
that have been overridden. The internal use ofthese functions is an implementation detail and users should not be able
to indirectly affect them this way.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
util console