console: make console consistent with the standard - overridable #12454

Wandalen · 2017-04-17T01:00:45Z

make console overridable by custom console( without delete )

add test suite from w3c/web-platform-tests( console-is-a-namespace )
few test cases commented out from the test suite
to make nodejs console even more consistent further changes needed
which will come in the next commit
which could be more breaking than this one

Fixes: #11805
Ref: https://console.spec.whatwg.org/#console-namespace
Ref: https://heycam.github.io/webidl/#es-namespaces

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)

TimothyGu · 2017-04-17T01:50:44Z

test/parallel/test-console-is-a-namespace.js

+  const prototype1 = Object.getPrototypeOf(console);
+  const prototype2 = Object.getPrototypeOf(prototype1);
+  // console.log('console',Object.getOwnPropertyNames(console));
+  // console.log('prototype1',Object.getOwnPropertyNames(prototype1));


Where did these two lines come from?

TimothyGu · 2017-04-17T01:52:34Z

test/parallel/test-console-is-a-namespace.js

+const common = require('../common');
+const assert = require('assert');
+const { test, assert_equals, assert_true, assert_false } = common.WPT;
+


Can you use the standard header for WPT tests?

Hi TinothyGu. Do you mean remove assert_false from the list?

Sorry for the confusion. I was referring to the "WPT Refs" block in the file I linked. Specifically, a permalink to the source of the file as well as a URL pointing to the license under which the WPT test is used should be included for compliance.

Thanks for explanation. Added.

TimothyGu · 2017-04-17T01:53:53Z

test/parallel/test-console-is-a-namespace.js

+                'The [[Prototype]]\'s [[Prototype]] must be Object Prototype');
+
+}, 'The prototype chain must be correct');
+


And please use the standard footer here, since the test below is not part of WPT.

Added /* eslint-enable */

TimothyGu · 2017-04-17T01:59:31Z

lib/internal/bootstrap_node.js

        return console;
+      },
+      set: function(customConsole) {
+        console = customConsole;


This assignment will invalidate the if (console) check above in the getter, in the case that global.console is deliberately set to any falsy value like global.console = undefined;. Instead, do the same thing you did in the getter and use Object.defineProperty() on global.

@TimothyGu not certain I understood you. global.console = undefined is completely legal operation according to the standard. @TimothyGu purpose of the PR is making nodejs console consistent with the standard. Do you understand it, right?

@TimothyGu removing set will make console not-overridable again. If you see a problem please explain it to find better solution than suggested by you.

What I meant was:

// Initially, the `console` variable is `undefined`, since console will be lazily // loaded in the getter. global.console = undefined; // global.console's setter is called; the `console` variable remains `undefined`. assert.strictEqual(global.console, undefined); // 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.

I see. Thinking about solution.

Suggested solution. Good point actually. I added a separate test suite specifically for the problem. It sets global.console = undefined above requires. Otherwise something in requires read global.console.

@TimothyGu any further requests/insights?

TimothyGu · 2017-04-17T03:09:50Z

lib/internal/bootstrap_node.js

+          configurable: true,
+          writable: true,
+          enumerable: false,
+          value: console


customConsole

Oh mine. I'm sleepy. It's late here. Fixed.

Want to add more tests.

@TimothyGu added few more tests.

joyeecheung · 2017-04-17T06:30:23Z

test/parallel/test-console-is-a-namespace.js

+
+/* eslint-disable */
+/* WPT Refs:
+   https://github.com/w3c/web-platform-tests/blob/master/console/console-is-a-namespace.any.js


Better reference a specific commit hash here (40e451c instead of master) in case the file changes or turns into a 404

benjamingr · 2017-04-17T08:58:45Z

lib/internal/bootstrap_node.js

@@ -268,15 +268,29 @@
    var console;
    Object.defineProperty(global, 'console', {
      configurable: true,
-      enumerable: true,
+      enumerable: false,


These changes need to be semver-major as this might break user code, although hopefully very rarely.

Makes sense.

watilde · 2017-04-17T09:14:22Z

test/parallel/test-console-is-a-namespace.js

+   License: http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html
+*/
+
+assert.doesNotThrow(() => {


This test doesn't come from the WPT, and we don't use it in this test. I think we can get rid of it :)

watilde · 2017-04-17T09:15:15Z

test/parallel/test-console-is-a-namespace.js

+const assert = require('assert');
+const { test, assert_equals, assert_true, assert_false } = common.WPT;
+
+assert.doesNotThrow(() => {


Same with L23. We can get rid of this test from this file.

Hello @watilde

Do you mean exclude a duplicate?

@watilde I removed the duplicate. Also there is a comment, explaining the test case above is not from WPT. The first test case is useful. It fails with original version of booststrap_node.js. Do you suggest to move the first test case into separate test suite?

Oh you're right! It's better to keep the test case to verify the update, and we don't need to move it into a separate file since this test is related to namespace :)

watilde · 2017-04-17T14:40:10Z

test/parallel/test-console-assing-undefined.js

+
+// Initially, the `console` variable is `undefined`, since console will be
+// lazily loaded in the getter.
+


make jslint threw an error:

node/test/parallel/test-console-assing-undefined.js
1:1 error Mandatory module "common" must be loaded required-modules

Adding require common gives "'common' is assigned a value but never used". What would be appropriate resolution of the problem?

Simulating of usage or adding a directive?

Just don't assign it anywhere. Instead of const common = require('../common'); do require('../common');.

@aqrln Smart. Do you participate in OdesaJS this year?

@watilde fixed.

@Wandalen almost there :) The common module should be the first one that a test requires. Please see our guide on writing tests.

Do you participate in OdessaJS this year?

Yep.

@aqrln I shall share my experience too if organizers won't be mean :)

TimothyGu · 2017-04-17T19:11:50Z

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

addaleax · 2017-04-22T04:25:56Z

lib/internal/bootstrap_node.js

        return console;
+      },
+      set: function(customConsole) {


you could use a set(customConsole) { shorthand here (for the getter as well if you like) :)

@addaleax thank you for your advices. I used => shorthand as you advised in the recent commit.

@addaleax can/should I push that changes after TimothyGu launched remote testing?

@Wandalen go ahead and push it. The CI is done already (the "pending" windows-fanned instance is having some infrastructural issues). And we can always launch a new CI.

addaleax · 2017-04-22T04:26:30Z

lib/internal/bootstrap_node.js

+          writable: true,
+          enumerable: false,
+          value: console
+        });


could this maybe use the setter directly, i.e. as global.console = console?

@addaleax I tried that. global.console = console fails at test/common.js:407 with message

AssertionError: Unexpected global(s) found: console

You can fix that in test-console-is-a-namespace.js by calling common.allowGlobals('console').

Fail comes form

=== release console_low_stack_space === Path: message/console_low_stack_space Hello, World! assert.js:86 throw new assert.AssertionError({

I tried common.allowGlobals('console') in that file and 2 I create, it didn't help. Maybe I do something wrong.

refack · 2017-04-22T20:53:08Z

@Wandalen could you add a link to the standard in the first comment (I assume it's https://console.spec.whatwg.org/#console-namespace)

addaleax · 2017-04-29T12:52:07Z

@nodejs/ctc This would require another CTC approval, do you mind taking a look?

cjihrig · 2017-05-01T15:22:50Z

test/parallel/test-console-is-a-namespace.js

+
+// Tests above are not from WPT.
+
+/* eslint-disable */


If Node collaborators should not modify these tests in any way, please add a comment here stating that.

@cjihrig like that?

I would put the comment inside the existing WPT Refs: block comment and say something like "The following tests are copied from upstream verbatim. Do not modify."

@cjihrig done.

@cjihrig do you want any other improvement?

@cjihrig are you going approve my PR? Do you have any objection? Do you make all newcomers feel unwelcome here or it's personal?

@Wandalen ... this kind of comment is unwarranted. We are all busy and this repository has a high level of activity that is often difficult to keep track of. Many of us receive hundreds of notifications per day. Patience would be appreciated.

@jasnell could you please tell what would be an appropriate comment there? I caught another issue with ArrayBuffer. Looking forward to starting work on that issue once you will approve the PR. At the point, I clearly see that the majority of the community is friendly, but the minority make me feel unwelcome here.

@Wandalen try not to get frustrated, and really don't take it personally.
Initially I also had a hard time trying to deduce people's intention and emotions from just these text messages. It's hard, I misunderstood some stuff, I took it personally, and I got frustrated, and then angry, and that leads to nowhere good.
If you found another issue to channel your energy into, I say go for it. This PR will most probably land in due time.

@refack thank you for kind words.

Fishrock123 · 2017-05-01T15:34:50Z

I personally think all these specs make way too many things unenumberable without much reason, so... -1 on that? I guess?

Wandalen · 2017-05-01T15:41:30Z

@Fishrock123 on what? enumerability off or the whole propose?

Wandalen · 2017-05-01T15:51:50Z

@refack Yes.
https://console.spec.whatwg.org/#console-namespace
and
https://heycam.github.io/webidl/#es-namespaces

Trott · 2017-08-16T03:52:51Z

I'd like to see this get across the finish line or, if that's not something that's going to happen, closed. So, I rebased, fixed a failing test, pulled in the latest/greatest from the WPT test suite for one test, and updated some comments.

PTAL!

@Fishrock123 Is your -1 a blocking -1 or just an expression of annoyance with standards bodies' fondness for making things unenumerable?

@cjihrig I think your changes-requested can be dismissed, but let me know if that's not right.

If anyone is all "Wait, what, no way! Don't do this!", now's a great time to register your opinion.

Update the test from the most recent WPT contents. Copyedit some existing comments.

cjihrig · 2017-08-16T22:05:10Z

test/parallel/test-console-is-a-namespace.js

+const self = global;
+
+/* eslint-disable */
+/* The following tests are copied from */


I'd still prefer if this said not to modify, not just that it's copied.

BridgeAR

LGTM but it would be great to have the comment nit addressed.

BridgeAR · 2017-08-28T01:38:20Z

When I just ran the tests for it again I realized that this is not working.

It works with the following patch

--- a/test/message/console_low_stack_space.js
+++ b/test/message/console_low_stack_space.js
@@ -16,7 +16,7 @@ function a() {
   try {
     return a();
   } catch (e) {
-    compiledConsole = consoleDescriptor.get();
+    compiledConsole = consoleDescriptor.value;
     if (compiledConsole.log) {
       // Using `console.log` itself might not succeed yet, but the code for it
       // has been compiled.

Addressing the nit

diff --git a/test/parallel/test-console-is-a-namespace.js b/test/parallel/test-console-is-a-namespace.js
index 1beb392621..28b0668f41 100644
--- a/test/parallel/test-console-is-a-namespace.js
+++ b/test/parallel/test-console-is-a-namespace.js
@@ -13,7 +13,7 @@ assert.doesNotThrow(() => {
 const self = global;
 
 /* eslint-disable */
-/* The following tests are copied from */
+/* The following tests should not be modified as they are copied from */
 /* 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

I would also like to add the following patch because I think it is a bad idea to add commented code to core. Instead it should be added when necessary.

diff --git a/test/parallel/test-console-is-a-namespace.js b/test/parallel/test-console-is-a-namespace.js
index f413000805..1beb392621 100644
--- a/test/parallel/test-console-is-a-namespace.js
+++ b/test/parallel/test-console-is-a-namespace.js
@@ -38,12 +38,4 @@ 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 */

Wandalen · 2017-08-28T04:43:57Z

Hello @BridgeAR. What test cases does it break? Why did you added enumerable: true? I believe there should be enumerable: false.

BridgeAR · 2017-08-29T00:13:24Z

I did not change the console to be enumerable (your test case would also fail in that case) and you actually struggled with the test case that I fixed. The default for enumerable is false that is why I did not declare it in my former clean up. Anyhow, I removed the code cleanup from my patches above as I am going to open a broader clean up after this PR. The tests failed because global.console gets redefined with a value and has no getter after the first call and therefore the test failed and the get() had to be changed to value.

So please feel free to add my patches so this can land. @Wandalen

BridgeAR

The test must be fixed and I would like the other two comments addressed as well.

BridgeAR · 2017-09-20T00:29:28Z

Ping @Wandalen this now needs a rebase.

BridgeAR · 2017-09-24T17:26:19Z

Closing this due to the long inactivity. @Wandalen please leave a comment if you would like to pursue this further.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 17, 2017

TimothyGu suggested changes Apr 17, 2017

View reviewed changes

TimothyGu added console Issues and PRs related to the console subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 17, 2017

TimothyGu suggested changes Apr 17, 2017

View reviewed changes

TimothyGu approved these changes Apr 17, 2017

View reviewed changes

TimothyGu mentioned this pull request Apr 17, 2017

console - writable property #11805

Closed

joyeecheung reviewed Apr 17, 2017

View reviewed changes

benjamingr reviewed Apr 17, 2017

View reviewed changes

benjamingr approved these changes Apr 17, 2017

View reviewed changes

watilde reviewed Apr 17, 2017

View reviewed changes

benjamingr added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 17, 2017

watilde reviewed Apr 17, 2017

View reviewed changes

jasnell requested review from bnoordhuis, addaleax and Fishrock123 April 17, 2017 17:39

addaleax approved these changes Apr 22, 2017

View reviewed changes

watilde approved these changes Apr 22, 2017

View reviewed changes

TimothyGu added dont-land-on-v4.x labels Apr 22, 2017

refack approved these changes Apr 22, 2017

View reviewed changes

cjihrig requested changes May 1, 2017

View reviewed changes

Wandalen added 13 commits August 15, 2017 16:41

console : remove diagnostic information output

e10a6dd

console : fix typo

28c32a2

console : few more test cases

02768c0

console : stadard remove redundandt test case

5f9d30f

console : reference specific commit instead of master in test suite

bb26718

console : fix jsling issue

352d504

console : reverse requires in test as requested

197ec07

console : => shorthand

c653812

console : requested comment

5a411bc

console : requested comment

c9c572a

console : requested comment

b553b49

console : requested comment

b7d9771

console : requested comment

8023728

Trott force-pushed the console-consistent-with-standard branch from 98388d5 to 662a8ba Compare August 16, 2017 03:47

squash: update test-console-is-a-namespace

490374f

Update the test from the most recent WPT contents. Copyedit some existing comments.

Trott force-pushed the console-consistent-with-standard branch from 662a8ba to 490374f Compare August 16, 2017 04:01

squash: rename test

2f43d8d

cjihrig approved these changes Aug 16, 2017

View reviewed changes

TimothyGu approved these changes Aug 17, 2017

View reviewed changes

BridgeAR approved these changes Aug 26, 2017

View reviewed changes

BridgeAR self-assigned this Aug 28, 2017

BridgeAR requested changes Aug 29, 2017

View reviewed changes

BridgeAR closed this Sep 24, 2017

BridgeAR mentioned this pull request Dec 16, 2017

console: make console more standard compliant #17708

Closed

4 tasks

		'The [[Prototype]]\'s [[Prototype]] must be Object Prototype');

		}, 'The prototype chain must be correct');


		// Initially, the `console` variable is `undefined`, since console will be
		// lazily loaded in the getter.

console: make console consistent with the standard - overridable #12454

console: make console consistent with the standard - overridable #12454

Conversation

Wandalen commented Apr 17, 2017 • edited by BridgeAR Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wandalen Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wandalen Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wandalen Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Apr 22, 2017

addaleax commented Apr 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wandalen May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Wandalen May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented May 1, 2017 • edited Loading

Wandalen commented May 1, 2017

Wandalen commented May 1, 2017

Trott commented Aug 16, 2017

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Wandalen commented Apr 17, 2017 •

edited by BridgeAR

Loading

Wandalen Apr 17, 2017 •

edited

Loading

Wandalen Apr 17, 2017 •

edited

Loading

Wandalen Apr 22, 2017 •

edited

Loading

TimothyGu Apr 22, 2017 •

edited

Loading

cjihrig May 1, 2017 •

edited

Loading

Wandalen May 1, 2017 •

edited

Loading

Wandalen May 11, 2017 •

edited

Loading

Fishrock123 commented May 1, 2017 •

edited

Loading

BridgeAR commented Aug 28, 2017 •

edited

Loading

BridgeAR left a comment •

edited

Loading