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

console: make console more standard compliant #17708

Closed
wants to merge 3 commits into from

Conversation

BridgeAR
Copy link
Member

According to the standard the property descriptor of console
should be writable and non-enumerable.

Refs #12454
Fixes: #11805

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

console

@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 16, 2017
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 16, 2017
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.
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@Trott
Copy link
Member

Trott commented Dec 16, 2017

pinging previous approvers: @TimothyGu @refack @bnoordhuis @addaleax @benjamingr @watilde @cjihrig

pinging author of first commit: @Wandalen

@@ -354,10 +352,9 @@
originalConsole[key],
wrappedConsole[key],
config);
delete originalConsole[key];
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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

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.
Copy link
Member

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.

Copy link
Member Author

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.

@BridgeAR BridgeAR force-pushed the console-standard branch 2 times, most recently from 44ea72f to 965146c Compare December 16, 2017 18:35
@BridgeAR
Copy link
Member Author

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.

@BridgeAR
Copy link
Member Author

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 18, 2017
@BridgeAR BridgeAR requested a review from a team December 18, 2017 16:35
@BridgeAR
Copy link
Member Author

@nodejs/tsc PTAL. This needs more LGs.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
Copy link
Member Author

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

I separated the code from @Wandalen and me.

Copy link
Member

@apapirovski apapirovski left a 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.

@@ -354,10 +352,9 @@
originalConsole[key],
wrappedConsole[key],
config);
delete originalConsole[key];
Copy link
Member

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?

@apapirovski
Copy link
Member

Also, FWIW, I think the check is very likely faster than the delete. The latter is very expensive in my experience.

@BridgeAR
Copy link
Member Author

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

Wandalen and others added 2 commits January 17, 2018 21:47
This imports a standard test from w3c/web-platform-tests
(console-is-a-namespace).
@BridgeAR
Copy link
Member Author

BridgeAR commented Jan 17, 2018

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 18, 2018
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>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 18, 2018
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>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 18, 2018
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>
@BridgeAR
Copy link
Member Author

Landed in f054855...7809f38

@BridgeAR BridgeAR closed this Jan 18, 2018
kfarnung added a commit that referenced this pull request Apr 24, 2018
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>
MylesBorins pushed a commit that referenced this pull request May 4, 2018
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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>
@BridgeAR BridgeAR deleted the console-standard branch April 1, 2019 23:38
teamdandelion added a commit to teamdandelion/node-eval that referenced this pull request Jul 5, 2019
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
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. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants