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

lib: make the global console [[Prototype]] an empty object #23509

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 12, 2018

lib: make the global console [[Prototype]] an empty object

From the WHATWG console spec:

For historical web-compatibility reasons, the namespace object for
console must have as its [[Prototype]] an empty object, created as
if by ObjectCreate(%ObjectPrototype%), instead of %ObjectPrototype%.

Since in Node.js, the Console constructor has been exposed through
require('console'), we need to keep the Console constructor but
we cannot actually use new Console to construct the global console.

This patch changes the prototype chain of the global console object,
so the console.Console.prototype is not in the global console prototype
chain anymore.

const proto = Object.getPrototypeOf(global.console);
// Before this patch
proto.constructor === global.console.Console
// After this patch
proto.constructor === Object

But, we still maintain that

global.console instanceof global.console.Console

through a custom Symbol.hasInstance function of Console that tests
for a special symbol kIsConsole for backwards compatibility.

This fixes a case in the console Web Platform Test that we commented
out.

Refs: https://console.spec.whatwg.org/#console-namespace
Refs: whatwg/console#3

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Oct 12, 2018
@joyeecheung joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 12, 2018
@joyeecheung
Copy link
Member Author

Defensively marked as semver-major. cc @nodejs/tsc

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

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

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

I’d like to measure userland breakage though, in case anyone is depending on console instanceof console.Console being true. If so maybe we can have a custom Symbol.hasInstance function to mitigate that.

@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

(Previous CITGM failed because disk on test-osuosl-aix61-ppc64_be-2 was full)
Master CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1585/
PR CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1586/

lib/console.js Outdated
ConsoleImpl.call(this, ...args);
}

// Reflect.ownKeys() is used here for for retrieving Symbols
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Double for

lib/console.js Outdated
function noop() {}
function Console(...args) {
if (!(this instanceof Console)) {
return new Console(...arguments);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ...args would be better. I prefer to avoid arguments as much as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a copy-paste but yeah probably also makes sense to change it in this patch

@jdalton
Copy link
Member

jdalton commented Oct 15, 2018

I've seen interesting Console use in jest, I don't have the specifics handy though. I suspect there may be breaks for a few so the major bump makes sense.

lib/console.js Outdated
// we cannot actually use `new Console` to construct the global console.
// Therefore, the console.Console.prototype is not
// in the global console prototype chain anymore.
const consolePrototype = Object.create(Object.prototype);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't Object.create(Object.prototype) just {} ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have an opinion on this, it just somehow seems more readable to be if we just do exactly what the spec says (but probably {} is more readable for people who skip the comments).

@thefourtheye
Copy link
Contributor

@jdalton Then it would make sense to include jest in CITGM if it is not there already. cc @nodejs/citgm

@richardlau
Copy link
Member

@jdalton Then it would make sense to include jest in CITGM if it is not there already. cc @nodejs/citgm

Currently blocked because Jest uses yarn for tests: nodejs/citgm#560

lib/console.js Outdated
function noop() {}
function Console(...args) {
if (!(this instanceof Console)) {
return new Console(...arguments);
Copy link
Member

Choose a reason for hiding this comment

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

Could use Reflect.construct(Console, args)

lib/console.js Outdated

function noop() {}
function Console(...args) {
if (!(this instanceof Console)) {
Copy link
Member

Choose a reason for hiding this comment

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

instead of !(this instanceof Console) could use !new.target

Copy link
Member

Choose a reason for hiding this comment

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

Not that anybody does that, but these would behave differently if somebody did ES5-style inheritance from this class, right? The current version is also a bit more idiomatic, imo (same for the comment above…)

@jdalton
Copy link
Member

jdalton commented Oct 15, 2018

I'm a little fuzzy on the need for ConsoleImpl. It looks like the gist of this PR is to make the global console no longer an instance of Console which should be doable, by copying the own and inherited props of a new Console(stdout, stderr) instance (minus constructor) to an empty object {}, without rewiring Console to use an internal ConsoleImpl.

@joyeecheung
Copy link
Member Author

by copying the own and inherited props of a new Console(stdout, stderr) instance (minus constructor) to an empty object {} without rewiring Console to use an internal ConsoleImpl.

I am a bit conflicted on the two approach (adding props to an object v.s. removing props from a Console). One upside about using the name ConsoleImpl is so that we don't have to put a bunch of comments to remind whoever wants to modify the file later that the global console is not actually an instance of Console - the code there are just implementations that will be wired to the global console. Somehow it seems to me that if we go with removing things, then we could accidentally forget to remove something added later, but I don't have strong opinions on this, it's just a hunch.

@jdalton
Copy link
Member

jdalton commented Oct 16, 2018

I think this is overcomplicating the PR. Here's a snippet from my own implementation in a related project.

const globalConsole = assignProperties({}, new Console(stdout, stderr))

There is no need to rewire Console or abstract through an intermediate ConsoleImpl. The line above accomplishes the goal of making globalConsole assigned to a plain object without a heavy rewrite. The assignProperties util is doing a loop over Reflect.ownKeys() and Reflect.defineProperty(o, k, Reflect.getOwnPropertyDescriptor(s, k))

@devsnek
Copy link
Member

devsnek commented Oct 16, 2018

personally i would do this by having a ConsolePrototype object which is used with Object.create like in our ReadableStreamAsyncIterator implementation. https://github.com/nodejs/node/blob/master/lib/internal/streams/async_iterator.js#L130

nvm i realize that it has to be a plain object. current implementation seems fine.

Copy link
Member

@jdalton jdalton left a comment

Choose a reason for hiding this comment

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

Please look into simplifying this PR. I believe the gist of the PR can be done in a handful of lines without major refactoring to Console. See #23509 (comment).

@joyeecheung
Copy link
Member Author

@jdalton

What do you think about #23509 (comment) ? So far 33 contributors have committed in this file, so I believe it's worth the cost to rename the class to hint future contributors that this is an implementation without having them to scroll all the way down to see the comments.

@jdalton
Copy link
Member

jdalton commented Oct 16, 2018

@joyeecheung

What do you think about #23509 (comment) ?

Class renaming, or other deeper refactoring, is probably better suited for a PR specifically for that.
This PR, for a WHATWG console spec compliance fix, is less great for that.

@Trott
Copy link
Member

Trott commented Oct 16, 2018

Class renaming, or other deeper refactoring, is probably better suited for a PR specifically for that.
This PR, for a WHATWG console spec compliance fix, is less great for that.

@jdalton I don't think the question was if we should do refactoring in this PR. I think the question was what you thought about the argument made in that comment that the approach currently in this PR is more maintainable in that it will be less error-prone for future contributors to modify. Here's the comment:

One upside about using the name ConsoleImpl is so that we don't have to put a bunch of comments to remind whoever wants to modify the file later that the global console is not actually an instance of Console - the code there are just implementations that will be wired to the global console. Somehow it seems to me that if we go with removing things, then we could accidentally forget to remove something added later, but I don't have strong opinions on this, it's just a hunch.

@jdalton
Copy link
Member

jdalton commented Oct 16, 2018

@Trott

I think the question was what you thought about the argument made in that comment that the approach currently in this PR is more maintainable in that it will be less error-prone for future contributors to modify.

Oh, I don't think the approach in this PR promotes those things.
I'd be against it as is, but wouldn't be against further iterating on it in a more focused PR.

@SimenB
Copy link
Member

SimenB commented Oct 22, 2018

Jest team member here 👋 Saw the link from the CITGM PR, and in Jest we do class OurConsole extends Console and super.log('stuff'). I haven't looked at the code in this PR, but do you think that would break? If it's just changing instanceof I don't think that matters to us.

Code:
https://github.com/facebook/jest/blob/cdf7a222420ae2ad21c76f15dd155e2ebacad79d/packages/jest-util/src/Console.js
https://github.com/facebook/jest/blob/cdf7a222420ae2ad21c76f15dd155e2ebacad79d/packages/jest-util/src/buffered_console.js

An instance of that subclass is injected into the user code (running as a vm.Script), overwriting their global.console. We do this both so we can buffer up console output so it doesn't interfere with our own output, so we can give a line and column of where the log came from, and so we can pass it to reporters.

@joyeecheung
Copy link
Member Author

@SimenB Hi! Thanks for the explanation, based on that I don't think this would break Jest, since after this patch the lookups on the global console and other Console instances still arrive to the same stuff - just a matter of through the prototype chain or not.

@trygve-lie
Copy link

I have a module where this will be breaking: https://github.com/trygve-lie/abslog/blob/master/lib/log.js#L8

The module is an abstract log API making it possible for modules to have log statements without depending on a full log library. A consumer of a module using this abstraction are then able to pass in a logger of their choice as long it comply with the API of the abstraction. This is btw a pattern used in fastify.js, though this module differ from the one in fastify.js that this one support passing in console as an exception despite that it does not comply with the API.

If so maybe we can have a custom Symbol.hasInstance function to mitigate that.

Having something like that would be very handy.

@joyeecheung
Copy link
Member Author

@trygve-lie Thanks for the explanation! Indeed, we should implement a Symbol.hasInstance to mitigate that - I'll find sometime later this week to update this PR.

From the WHATWG console spec:

> For historical web-compatibility reasons, the namespace object for
> console must have as its [[Prototype]] an empty object, created as
> if by ObjectCreate(%ObjectPrototype%), instead of %ObjectPrototype%.

Since in Node.js, the Console constructor has been exposed through
require('console'), we need to keep the Console constructor but
we cannot actually use `new Console` to construct the global console.

This patch changes the prototype chain of the global console object,
so the console.Console.prototype is not in the global console prototype
chain anymore.

```
const proto = Object.getPrototypeOf(global.console);
// Before this patch
proto.constructor === global.console.Console
// After this patch
proto.constructor === Object
```

But, we still maintain that

```
global.console instanceof global.console.Console
```

through a custom Symbol.hasInstance function of Console that tests
for a special symbol kIsConsole for backwards compatibility.

This fixes a case in the console Web Platform Test that we commented
out.

Refs: https://console.spec.whatwg.org/#console-namespace
Refs: whatwg/console#3
@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 25, 2018

@jdalton I looked into the alternative implementation a bit, and noticed that simply assigning properties of a Console instance to a new object would result in a behavior change - the methods called on the global console will have the temporary Console instance as their contexts since the Console constructor binds all the method to the instance being constructed (so that users can do const { log } = console). So for instance, monkey patching properties of the global console could be broken:

console._stdout = undefined;
// Should've thrown because we have broken _stdout, but doesn't because log
// actually gets called on another object.
console.log('test'); 

In the context of the Web, this is expected because console is a namespace and stuff like console._stdout are not a thing, but in the context of Node.js, this is probably a bit too breaking - I am pretty sure people are monkey patching these properties in the wild, or assume that they carry the states as altered by the methods, so these are probably properties that we even need to think twice before starting a proper deprecation cycle. That's probably a bit too far away from the intention of this PR, though.

I've patched that through a condition in the loop to rebind the methods to the global console.

@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 25, 2018

Another note: I went with new.target in the constructor to implement Symbol.hasInstance, and added a few tests for the instanceof behavior.

CI: https://ci.nodejs.org/job/node-test-pull-request/18145/
Master CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1598/
PR CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1599/

@jdalton
Copy link
Member

jdalton commented Oct 25, 2018

@joyeecheung Thank you for the update on your findings and the PR!

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 25, 2018
@danbev
Copy link
Contributor

danbev commented Oct 26, 2018

Landed in 6223236.

@danbev danbev closed this Oct 26, 2018
danbev pushed a commit that referenced this pull request Oct 26, 2018
From the WHATWG console spec:

> For historical web-compatibility reasons, the namespace object for
> console must have as its [[Prototype]] an empty object, created as
> if by ObjectCreate(%ObjectPrototype%), instead of %ObjectPrototype%.

Since in Node.js, the Console constructor has been exposed through
require('console'), we need to keep the Console constructor but
we cannot actually use `new Console` to construct the global console.

This patch changes the prototype chain of the global console object,
so the console.Console.prototype is not in the global console prototype
chain anymore.

```
const proto = Object.getPrototypeOf(global.console);
// Before this patch
proto.constructor === global.console.Console
// After this patch
proto.constructor === Object
```

But, we still maintain that

```
global.console instanceof global.console.Console
```

through a custom Symbol.hasInstance function of Console that tests
for a special symbol kIsConsole for backwards compatibility.

This fixes a case in the console Web Platform Test that we commented
out.

PR-URL: #23509
Refs: whatwg/console#3
Refs: https://console.spec.whatwg.org/#console-namespace
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 9, 2019
From the WHATWG console spec:

> For historical web-compatibility reasons, the namespace object for
> console must have as its [[Prototype]] an empty object, created as
> if by ObjectCreate(%ObjectPrototype%), instead of %ObjectPrototype%.

Since in Node.js, the Console constructor has been exposed through
require('console'), we need to keep the Console constructor but
we cannot actually use `new Console` to construct the global console.

This patch changes the prototype chain of the global console object,
so the console.Console.prototype is not in the global console prototype
chain anymore.

```
const proto = Object.getPrototypeOf(global.console);
// Before this patch
proto.constructor === global.console.Console
// After this patch
proto.constructor === Object
```

But, we still maintain that

```
global.console instanceof global.console.Console
```

through a custom Symbol.hasInstance function of Console that tests
for a special symbol kIsConsole for backwards compatibility.

This fixes a case in the console Web Platform Test that we commented
out.

PR-URL: nodejs#23509
Refs: whatwg/console#3
Refs: https://console.spec.whatwg.org/#console-namespace
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 9, 2019
BridgeAR pushed a commit that referenced this pull request Jan 9, 2019
From the WHATWG console spec:

> For historical web-compatibility reasons, the namespace object for
> console must have as its [[Prototype]] an empty object, created as
> if by ObjectCreate(%ObjectPrototype%), instead of %ObjectPrototype%.

Since in Node.js, the Console constructor has been exposed through
require('console'), we need to keep the Console constructor but
we cannot actually use `new Console` to construct the global console.

This patch changes the prototype chain of the global console object,
so the console.Console.prototype is not in the global console prototype
chain anymore.

```
const proto = Object.getPrototypeOf(global.console);
// Before this patch
proto.constructor === global.console.Console
// After this patch
proto.constructor === Object
```

But, we still maintain that

```
global.console instanceof global.console.Console
```

through a custom Symbol.hasInstance function of Console that tests
for a special symbol kIsConsole for backwards compatibility.

This fixes a case in the console Web Platform Test that we commented
out.

PR-URL: #23509
Refs: whatwg/console#3
Refs: https://console.spec.whatwg.org/#console-namespace
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jan 9, 2019
Specifically for v11.x.

PR-URL: #25420
Refs: #23509
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
From the WHATWG console spec:

> For historical web-compatibility reasons, the namespace object for
> console must have as its [[Prototype]] an empty object, created as
> if by ObjectCreate(%ObjectPrototype%), instead of %ObjectPrototype%.

Since in Node.js, the Console constructor has been exposed through
require('console'), we need to keep the Console constructor but
we cannot actually use `new Console` to construct the global console.

This patch changes the prototype chain of the global console object,
so the console.Console.prototype is not in the global console prototype
chain anymore.

```
const proto = Object.getPrototypeOf(global.console);
// Before this patch
proto.constructor === global.console.Console
// After this patch
proto.constructor === Object
```

But, we still maintain that

```
global.console instanceof global.console.Console
```

through a custom Symbol.hasInstance function of Console that tests
for a special symbol kIsConsole for backwards compatibility.

This fixes a case in the console Web Platform Test that we commented
out.

PR-URL: nodejs#23509
Refs: whatwg/console#3
Refs: https://console.spec.whatwg.org/#console-namespace
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Specifically for v11.x.

PR-URL: nodejs#25420
Refs: nodejs#23509
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. console Issues and PRs related to the console subsystem. 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.