console: make console consistent with the standard - overridable#12454
console: make console consistent with the standard - overridable#12454Wandalen wants to merge 21 commits intonodejs:masterfrom Wandalen:console-consistent-with-standard
Conversation
nodejs-github-bot
added
the
lib / src
There was a problem hiding this comment.
Where did these two lines come from?
There was a problem hiding this comment.
Hi TinothyGu. Do you mean remove assert_false from the list?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for explanation. Added.
There was a problem hiding this comment.
And please use the standard footer here, since the test below is not part of WPT.
There was a problem hiding this comment.
Added /* eslint-enable */
lib/internal/bootstrap_node.js
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
I see. Thinking about solution.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@TimothyGu any further requests/insights?
TimothyGu
added
console
lib/internal/bootstrap_node.js
Outdated
There was a problem hiding this comment.
Oh mine. I'm sleepy. It's late here. Fixed.
There was a problem hiding this comment.
Want to add more tests.
There was a problem hiding this comment.
Better reference a specific commit hash here (40e451c instead of master) in case the file changes or turns into a 404
lib/internal/bootstrap_node.js
Outdated
There was a problem hiding this comment.
These changes need to be semver-major as this might break user code, although hopefully very rarely.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Same with L23. We can get rid of this test from this file.
There was a problem hiding this comment.
Do you mean exclude a duplicate?
There was a problem hiding this comment.
There was a problem hiding this comment.
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 :)
benjamingr
added
the
semver-major
There was a problem hiding this comment.
make jslint threw an error:
node/test/parallel/test-console-assing-undefined.js
1:1 error Mandatory module "common" must be loaded required-modules
There was a problem hiding this comment.
Adding require common gives "'common' is assigned a value but never used". What would be appropriate resolution of the problem?
There was a problem hiding this comment.
Simulating of usage or adding a directive?
There was a problem hiding this comment.
Just don't assign it anywhere. Instead of const common = require('../common'); do require('../common');.
There was a problem hiding this comment.
@aqrln Smart. Do you participate in OdesaJS this year?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@aqrln I shall share my experience too if organizers won't be mean :)
lib/internal/bootstrap_node.js
Outdated
There was a problem hiding this comment.
you could use a set(customConsole) { shorthand here (for the getter as well if you like) :)
There was a problem hiding this comment.
@addaleax thank you for your advices. I used => shorthand as you advised in the recent commit.
There was a problem hiding this comment.
@addaleax can/should I push that changes after TimothyGu launched remote testing?
There was a problem hiding this comment.
@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.
lib/internal/bootstrap_node.js
Outdated
There was a problem hiding this comment.
could this maybe use the setter directly, i.e. as global.console = console?
There was a problem hiding this comment.
@addaleax I tried that. global.console = console fails at test/common.js:407 with message
AssertionError: Unexpected global(s) found: console
There was a problem hiding this comment.
You can fix that in test-console-is-a-namespace.js by calling common.allowGlobals('console').
There was a problem hiding this comment.
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.
|
@Wandalen could you add a link to the standard in the first comment (I assume it's https://console.spec.whatwg.org/#console-namespace) |
|
@nodejs/ctc This would require another CTC approval, do you mind taking a look? |
There was a problem hiding this comment.
If Node collaborators should not modify these tests in any way, please add a comment here stating that.
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
@cjihrig do you want any other improvement?
There was a problem hiding this comment.
@cjihrig are you going approve my PR? Do you have any objection? Do you make all newcomers feel unwelcome here or it's personal?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
|
I personally think all these specs make way too many things unenumberable without much reason, so... -1 on that? I guess? |
|
@Fishrock123 on what? enumerability off or the whole propose? |
If global.console write undefined would come earlier than global.console read then lazy read in get would happen, despite write was done. This commit fix the problem. Fixes: #11805
|
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.
| const self = global; | ||
|
|
||
| /* eslint-disable */ | ||
| /* The following tests are copied from */ |
There was a problem hiding this comment.
I'd still prefer if this said not to modify, not just that it's copied.
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
LGTM but it would be great to have the comment nit addressed.
|
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.htmlI 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 */ |
|
Hello @BridgeAR. What test cases does it break? Why did you added enumerable: true? I believe there should be enumerable: false. |
|
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 So please feel free to add my patches so this can land. @Wandalen |
|
Ping @Wandalen this now needs a rebase. |
|
Closing this due to the long inactivity. @Wandalen please leave a comment if you would like to pursue this further. |
16 participants
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), orvcbuild test(Windows) passesAffected core subsystem(s)