-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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 consistent with the standard - overridable #12454
Changes from all commits
ea237b2
d98c130
4a0669d
fad6725
6e86bca
79dc629
e10a6dd
28c32a2
02768c0
5f9d30f
bb26718
352d504
197ec07
c653812
5a411bc
c9c572a
b553b49
b7d9771
8023728
490374f
2f43d8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -270,14 +270,28 @@ | |
let console; | ||
Object.defineProperty(global, 'console', { | ||
configurable: true, | ||
enumerable: true, | ||
get: function() { | ||
enumerable: false, | ||
get: () => { | ||
if (!console) { | ||
console = (originalConsole === undefined) ? | ||
NativeModule.require('console') : | ||
installInspectorConsole(originalConsole); | ||
} | ||
Object.defineProperty(global, 'console', { | ||
configurable: true, | ||
writable: true, | ||
enumerable: false, | ||
value: console | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could this maybe use the setter directly, i.e. as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addaleax I tried that. global.console = console fails at test/common.js:407 with message
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can fix that in test-console-is-a-namespace.js by calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fail comes form
I tried common.allowGlobals('console') in that file and 2 I create, it didn't help. Maybe I do something wrong. |
||
return console; | ||
}, | ||
set: (customConsole) => { | ||
Object.defineProperty(global, 'console', { | ||
configurable: true, | ||
writable: true, | ||
enumerable: false, | ||
value: customConsole | ||
}); | ||
} | ||
}); | ||
setupInspectorCommandLineAPI(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
'use strict'; | ||
|
||
// Should be above require, because code in require read console | ||
// what we are trying to avoid | ||
// set should be earlier than get | ||
|
||
global.console = undefined; | ||
|
||
// Initially, the `console` variable is `undefined`, since console will be | ||
// lazily loaded in the getter. | ||
|
||
require('../common'); | ||
const assert = require('assert'); | ||
|
||
// 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. | ||
|
||
assert.strictEqual(global.console, undefined, 'first read'); | ||
assert.strictEqual(global.console, undefined, 'second read'); | ||
|
||
global.console = 1; | ||
assert.strictEqual(global.console, 1, 'set true-like primitive'); | ||
|
||
global.console = 0; | ||
assert.strictEqual(global.console, 0, 'set false-like primitive, again'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
'use strict'; | ||
|
||
require('../common'); | ||
|
||
const assert = require('assert'); | ||
const { test, assert_equals, assert_true, assert_false } = | ||
require('../common/wpt'); | ||
|
||
assert.doesNotThrow(() => { | ||
global.console = global.console; | ||
}); | ||
|
||
const self = global; | ||
|
||
/* eslint-disable */ | ||
/* The following tests are copied from */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd still prefer if this said not to modify, not just that it's copied. |
||
/* WPT Refs: | ||
https://github.com/w3c/web-platform-tests/blob/40e451c/console/console-is-a-namespace.any.js | ||
License: http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html | ||
*/ | ||
|
||
// https://heycam.github.io/webidl/#es-namespaces | ||
// https://console.spec.whatwg.org/#console-namespace | ||
|
||
test(() => { | ||
assert_true(self.hasOwnProperty("console")); | ||
}, "console exists on the global object"); | ||
|
||
test(() => { | ||
const propDesc = Object.getOwnPropertyDescriptor(self, "console"); | ||
assert_equals(propDesc.writable, true, "must be writable"); | ||
assert_equals(propDesc.enumerable, false, "must not be enumerable"); | ||
assert_equals(propDesc.configurable, true, "must be configurable"); | ||
assert_equals(propDesc.value, console, "must have the right value"); | ||
}, "console has the right property descriptors"); | ||
|
||
test(() => { | ||
assert_false("Console" in self); | ||
}, "Console (uppercase, as if it were an interface) must not exist"); | ||
|
||
|
||
// test(() => { | ||
// const prototype1 = Object.getPrototypeOf(console); | ||
// const prototype2 = Object.getPrototypeOf(prototype1); | ||
|
||
// 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"); | ||
/* eslint-enable */ |
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.
customConsole
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.
Oh mine. I'm sleepy. It's late here. Fixed.
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.
Want to add more tests.
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.
@TimothyGu added few more tests.