Skip to content

Commit

Permalink
lib: make the global console [[Prototype]] an empty object
Browse files Browse the repository at this point in the history
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 <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
  • Loading branch information
joyeecheung authored and danbev committed Oct 26, 2018
1 parent 817e2e8 commit 6223236
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 40 deletions.
56 changes: 50 additions & 6 deletions lib/console.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,17 +60,21 @@ let cliTable;

// Track amount of indentation required via `console.group()`.
const kGroupIndent = Symbol('kGroupIndent');

const kFormatForStderr = Symbol('kFormatForStderr');
const kFormatForStdout = Symbol('kFormatForStdout');
const kGetInspectOptions = Symbol('kGetInspectOptions');
const kColorMode = Symbol('kColorMode');
const kIsConsole = Symbol('kIsConsole');

function Console(options /* or: stdout, stderr, ignoreErrors = true */) {
if (!(this instanceof Console)) {
// We have to test new.target here to see if this function is called
// with new, because we need to define a custom instanceof to accommodate
// the global console.
if (!new.target) {
return new Console(...arguments);
}

this[kIsConsole] = true;
if (!options || typeof options.write === 'function') {
options = {
stdout: options,
Expand Down Expand Up @@ -125,7 +129,7 @@ function Console(options /* or: stdout, stderr, ignoreErrors = true */) {
var keys = Object.keys(Console.prototype);
for (var v = 0; v < keys.length; v++) {
var k = keys[v];
this[k] = this[k].bind(this);
this[k] = Console.prototype[k].bind(this);
}

This comment has been minimized.

Copy link
@jdalton

jdalton Nov 1, 2018

Member

☝️ This breaks BufferConsole in jest-util because it extends console.Console but provides its own log and other methods. Since the line above changes this[k].bind(this) to Console.prototype[k].bind(this) the BufferConsole.prototype.log is not used.

This comment has been minimized.

Copy link
@Trott

Trott Nov 2, 2018

Member

@jdalton What do you recommend we do, if anything, to remedy?

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Nov 2, 2018

Author Member

I think this line can just be reverted - it came from the original implementation but doesn't really matter in the current implementation - also we should add a comment here about this compatibility risk

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Nov 2, 2018

Author Member

Although, it doesn't necessarily have to be us to fix this, since the user can do this loop after they call super as well instead of relying on the behavior of this constructor, if they do understand that this class is constructing an namespace instead of a general object (so overriding in the prototype chain is not supposed to be how to get the extension done) - but it doesn't really hurt to keep it this way.

This comment has been minimized.

Copy link
@jdalton

jdalton Nov 2, 2018

Member

In my own implementation I handled it by reverting the line back to this[k] = this[k].bind(this) and creating the globalConsole like:

  const globalConsole = new Console(stdout, stderr)

  for (const name of Reflect.ownKeys(Console.prototype)) {
    if (! has(globalConsole, name)) {
      copyProperty(globalConsole, Console.prototype, name)
    }
  }

  Reflect.setPrototypeOf(globalConsole, {})
}

Expand Down Expand Up @@ -465,10 +469,50 @@ Console.prototype.table = function(tabularData, properties) {
return final(keys, values);
};

module.exports = new Console({
function noop() {}

// See https://console.spec.whatwg.org/#console-namespace
// > 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.
// Therefore, the console.Console.prototype is not
// in the global console prototype chain anymore.
const globalConsole = Object.create({});
const tempConsole = new Console({
stdout: process.stdout,
stderr: process.stderr
});
module.exports.Console = Console;

function noop() {}
// Since Console is not on the prototype chain of the global console,
// the symbol properties on Console.prototype have to be looked up from
// the global console itself.
for (const prop of Object.getOwnPropertySymbols(Console.prototype)) {
globalConsole[prop] = Console.prototype[prop];
}

// Reflect.ownKeys() is used here for retrieving Symbols
for (const prop of Reflect.ownKeys(tempConsole)) {
const desc = { ...(Reflect.getOwnPropertyDescriptor(tempConsole, prop)) };
// Since Console would bind method calls onto the instance,
// make sure the methods are called on globalConsole instead of
// tempConsole.
if (typeof Console.prototype[prop] === 'function') {
desc.value = Console.prototype[prop].bind(globalConsole);
}
Reflect.defineProperty(globalConsole, prop, desc);
}

globalConsole.Console = Console;

Object.defineProperty(Console, Symbol.hasInstance, {
value(instance) {
return instance[kIsConsole];
}
});

module.exports = globalConsole;
79 changes: 48 additions & 31 deletions test/parallel/test-console-instance.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
const common = require('../common');
const assert = require('assert');
const Stream = require('stream');
const Console = require('console').Console;
const requiredConsole = require('console');
const Console = requiredConsole.Console;

const out = new Stream();
const err = new Stream();
Expand All @@ -35,6 +36,11 @@ process.stdout.write = process.stderr.write = common.mustNotCall();
// Make sure that the "Console" function exists.
assert.strictEqual('function', typeof Console);

assert.strictEqual(requiredConsole, global.console);
// Make sure the custom instanceof of Console works
assert.ok(global.console instanceof Console);
assert.ok(!({} instanceof Console));

// Make sure that the Console constructor throws
// when not given a writable stream instance.
common.expectsError(
Expand Down Expand Up @@ -62,46 +68,57 @@ common.expectsError(

out.write = err.write = (d) => {};

const c = new Console(out, err);
{
const c = new Console(out, err);
assert.ok(c instanceof Console);

out.write = err.write = common.mustCall((d) => {
assert.strictEqual(d, 'test\n');
}, 2);
out.write = err.write = common.mustCall((d) => {
assert.strictEqual(d, 'test\n');
}, 2);

c.log('test');
c.error('test');
c.log('test');
c.error('test');

out.write = common.mustCall((d) => {
assert.strictEqual(d, '{ foo: 1 }\n');
});
out.write = common.mustCall((d) => {
assert.strictEqual(d, '{ foo: 1 }\n');
});

c.dir({ foo: 1 });
c.dir({ foo: 1 });

// Ensure that the console functions are bound to the console instance.
let called = 0;
out.write = common.mustCall((d) => {
called++;
assert.strictEqual(d, `${called} ${called - 1} [ 1, 2, 3 ]\n`);
}, 3);
// Ensure that the console functions are bound to the console instance.
let called = 0;
out.write = common.mustCall((d) => {
called++;
assert.strictEqual(d, `${called} ${called - 1} [ 1, 2, 3 ]\n`);
}, 3);

[1, 2, 3].forEach(c.log);
[1, 2, 3].forEach(c.log);
}

// Console() detects if it is called without `new` keyword.
Console(out, err);
// Test calling Console without the `new` keyword.
{
const withoutNew = Console(out, err);
assert.ok(withoutNew instanceof Console);
}

// Extending Console works.
class MyConsole extends Console {
hello() {}
// Test extending Console
{
class MyConsole extends Console {
hello() {}
}
const myConsole = new MyConsole(process.stdout);
assert.strictEqual(typeof myConsole.hello, 'function');
assert.ok(myConsole instanceof Console);
}
const myConsole = new MyConsole(process.stdout);
assert.strictEqual(typeof myConsole.hello, 'function');

// Instance that does not ignore the stream errors.
const c2 = new Console(out, err, false);
{
const c2 = new Console(out, err, false);

out.write = () => { throw new Error('out'); };
err.write = () => { throw new Error('err'); };
out.write = () => { throw new Error('out'); };
err.write = () => { throw new Error('err'); };

assert.throws(() => c2.log('foo'), /^Error: out$/);
assert.throws(() => c2.warn('foo'), /^Error: err$/);
assert.throws(() => c2.dir('foo'), /^Error: out$/);
assert.throws(() => c2.log('foo'), /^Error: out$/);
assert.throws(() => c2.warn('foo'), /^Error: err$/);
assert.throws(() => c2.dir('foo'), /^Error: out$/);
}
4 changes: 1 addition & 3 deletions test/parallel/test-whatwg-console-is-a-namespace.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ test(() => {
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.
// assert_equals(Object.getOwnPropertyNames(prototype1).length, 0, "The [[Prototype]] must have no properties");
assert_equals(Object.getOwnPropertyNames(prototype1).length, 0, "The [[Prototype]] must have no properties");
assert_equals(prototype2, Object.prototype, "The [[Prototype]]'s [[Prototype]] must be %ObjectPrototype%");
}, "The prototype chain must be correct");

Expand Down

0 comments on commit 6223236

Please sign in to comment.