console: make console more standard compliant by BridgeAR · Pull Request #17708 · nodejs/node · GitHub

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

Nit: Even though the test isn't ours, this comment is, right? If so, maybe wrap it at 80 chars?

Sure

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

pinging author of first commit: @Wandalen

Why was this added and why were the two lines below removed?

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.

Hmm, I'm not sure what effect deleting originalConsole[key] could have. Is it really safe?

Cryptic comment. Maybe this?

Patch global.console before importing modules that may modify the console object.

Incomprehensible comment... I'd remove it.

After reading it... I totally agree. I have no idea what it stands for.

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.

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

@nodejs/tsc PTAL. This needs more LGs.

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

I separated the code from @Wandalen and me.

LGTM but the one delete statement flagged by @bnoordhuis does concern me.

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 delete. The latter is very expensive in my experience.

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

New CI https://ci.nodejs.org/job/node-test-pull-request/12590/
CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1207/ (broken CITGM)

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>

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

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 added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 16, 2017

Trott reviewed Dec 16, 2017

View reviewed changes

benjamingr approved these changes Dec 16, 2017

View reviewed changes

bnoordhuis reviewed Dec 16, 2017

View reviewed changes

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

jasnell approved these changes Dec 18, 2017

View reviewed changes

BridgeAR added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Dec 18, 2017

BridgeAR requested a review from a team December 18, 2017 16:35

targos approved these changes Dec 21, 2017

View reviewed changes

mcollina approved these changes Dec 21, 2017

View reviewed changes

lib: make console writable and non-enumerable

e6b42ee

According to the standard the property descriptor of console should be writable and non-enumerable. Fixes: nodejs#11805

BridgeAR force-pushed the console-standard branch from 965146c to 8388836 Compare January 17, 2018 00:24

BridgeAR requested a review from apapirovski January 17, 2018 20:34

apapirovski approved these changes Jan 17, 2018

View reviewed changes

Wandalen and others added 2 commits January 17, 2018 21:47

test: add standard console tests

b7ce7b3

This imports a standard test from w3c/web-platform-tests (console-is-a-namespace).

test: improve console tests

1193065

BridgeAR force-pushed the console-standard branch from 8388836 to 1193065 Compare January 17, 2018 20:47

jasnell approved these changes Jan 18, 2018

View reviewed changes

BridgeAR closed this Jan 18, 2018

SimenB mentioned this pull request Apr 19, 2018

Cannot extend Console in node 10 rc.0 #20157

Closed

kfarnung mentioned this pull request Apr 21, 2018

lib: make sure console is writable #20185

Closed

2 tasks

BridgeAR deleted the console-standard branch April 1, 2019 23:38

rensbaardman mentioned this pull request Jun 11, 2019

Modifying globals within module leaks to global with Node >=10 jhnns/rewire#167

Closed

teamdandelion mentioned this pull request Jul 5, 2019

ensure that globals includes process and console pierrec/node-eval#18

Merged

console: make console more standard compliant by BridgeAR · Pull Request #17708 · nodejs/node · GitHub

Uh oh!

Uh oh!

console: make console more standard compliant - #17708

console: make console more standard compliant#17708BridgeAR wants to merge 3 commits intonodejs:masternodejs/node:masterfrom BridgeAR:console-standardBridgeAR/node:console-standardCopy head branch name to clipboard

Conversation

BridgeAR commented Dec 16, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 16, 2017

Uh oh!

BridgeAR commented Dec 18, 2017

Uh oh!

BridgeAR commented Dec 21, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 21, 2017

Uh oh!

BridgeAR commented Jan 17, 2018

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Jan 17, 2018

Uh oh!

BridgeAR commented Jan 17, 2018

Uh oh!

BridgeAR commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Jan 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

console: make console more standard compliant#17708
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:console-standard

BridgeAR commented Jan 17, 2018 •
edited
Loading