-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
console: make console more standard compliant #17708
Conversation
const prototype1 = Object.getPrototypeOf(console); | ||
const prototype2 = Object.getPrototypeOf(prototype1); | ||
|
||
// This got commented out from the original test because in Node.js all functions are declared on the prototype. |
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.
Nit: Even though the test isn't ours, this comment is, right? If so, maybe wrap it at 80 chars?
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.
Sure
pinging previous approvers: @TimothyGu @refack @bnoordhuis @addaleax @benjamingr @watilde @cjihrig pinging author of first commit: @Wandalen |
lib/internal/bootstrap_node.js
Outdated
@@ -354,10 +352,9 @@ | |||
originalConsole[key], | |||
wrappedConsole[key], | |||
config); | |||
delete originalConsole[key]; |
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.
Why was this added and why were the two lines below removed?
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.
Ah, this should probably be a individual commit. It is only about a few entries but it did not feel good that we checked all entries again instead of just removing them beforehand.
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.
Hmm, I'm not sure what effect deleting originalConsole[key]
could have. Is it really safe?
|
||
// Should be above require, because code in require read console | ||
// what we are trying to avoid | ||
// set should be earlier than get |
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.
Cryptic comment. Maybe this?
Patch
global.console
before importing modules that may modify the console object.
// global.console's getter is called | ||
// Since the `console` cache variable is `undefined` and therefore false-y, | ||
// the getter still calls NativeModule.require() and returns the object | ||
// obtained from it, instead of returning `undefined` as expected. |
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.
Incomprehensible comment... I'd remove it.
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.
After reading it... I totally agree. I have no idea what it stands for.
44ea72f
to
965146c
Compare
I addressed the comments and refactored the test as it did feel like it could need some further polishing. Originally I did not touch the version from @Wandalen to keep the authorship in tact. This way it diverges quite a lot from the original. @bnoordhuis I also separated the other bootstrapping part. It was just to minimize the necessary operations. Since delete is actually still slow, it might not yield any performance benefit but the opposite, so I am also totally fine with removing that part again. |
@nodejs/tsc PTAL. This needs more LGs. |
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.
LGTM
@Wandalen I do not feel comfortable landing this with my changes to the tests without your permission. Would you be so kind and have a look? A alternative would probably be to use the original code from @Wandalen and have a separate commit from me that changes the file accordingly. What do you all think about that? |
According to the standard the property descriptor of console should be writable and non-enumerable. Fixes: nodejs#11805
965146c
to
8388836
Compare
I separated the code from @Wandalen and me. |
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.
LGTM but the one delete
statement flagged by @bnoordhuis does concern me.
lib/internal/bootstrap_node.js
Outdated
@@ -354,10 +352,9 @@ | |||
originalConsole[key], | |||
wrappedConsole[key], | |||
config); | |||
delete originalConsole[key]; |
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.
Hmm, I'm not sure what effect deleting originalConsole[key]
could have. Is it really safe?
Also, FWIW, I think the check is very likely faster than the |
@apapirovski that is indeed probably true. It just felt more natural for me this way. I am also sure it is a safe thing to do but due to the concerns and since it is not necessary at all, I am going to remove that commit. |
This imports a standard test from w3c/web-platform-tests (console-is-a-namespace).
8388836
to
1193065
Compare
According to the standard the property descriptor of console should be writable and non-enumerable. PR-URL: nodejs#17708 Fixes: nodejs#11805 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
This imports a standard test from w3c/web-platform-tests (console-is-a-namespace). PR-URL: nodejs#17708 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
PR-URL: nodejs#17708 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Landed in f054855...7809f38 |
The code currently assumes that `console` is already writable, but that's only if it was previously defined as writable. If it hasn't already been defined then the default value is false. Refs: #17708 PR-URL: #20185 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
The code currently assumes that `console` is already writable, but that's only if it was previously defined as writable. If it hasn't already been defined then the default value is false. Refs: #17708 PR-URL: #20185 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
According to the standard the property descriptor of console should be writable and non-enumerable. PR-URL: nodejs#17708 Fixes: nodejs#11805 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
This imports a standard test from w3c/web-platform-tests (console-is-a-namespace). PR-URL: nodejs#17708 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
PR-URL: nodejs#17708 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
The code attaches global properties to the sandbox context by iterating over all the enumerable properties of `global`. However, in node v10, `console` switched [to being non-enmuerable][1]. This means that for users of this library with node>10, any `console.log`s in evaluated scripts will fail. This commit fixes this issue by manually attaching console to the sandbox (when globals are being used). A test has been added. Prior to the change to eval.js, the test would pass in node v8 but fail in v10 and v12. Also, the tests were already failing in v12, because in v12 `process` also became non-enumerable. I've applied a similar fix to `process` to ensure that it's always available too. [1]: nodejs/node#17708
According to the standard the property descriptor of console
should be writable and non-enumerable.
Refs #12454
Fixes: #11805
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
console