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

console - writable property #11805

Closed
Wandalen opened this issue Mar 11, 2017 · 14 comments
Closed

console - writable property #11805

Wandalen opened this issue Mar 11, 2017 · 14 comments
Labels
console Issues and PRs related to the console subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@Wandalen
Copy link
Contributor

Wandalen commented Mar 11, 2017

I am writing testing utilities and have trouble on nodejs platform ridding off unwanted output.
"console" is writable property in a browser. On nodejs "console's" methods could be rewritten iterating them, so making "console" non-writable does not give any insurance, but only make solving of my problem and similar more tedious. From my point of view having "console" non-writable counter-intuitive neither reasonable.

I propose to return writability to "console".

@addaleax addaleax added the console Issues and PRs related to the console subsystem. label Mar 11, 2017
@sam-github
Copy link
Contributor

Sounds OK to me, @Wandalen, are you going to PR this?

@sam-github sam-github added the feature request Issues that request new features to be added to Node.js. label Mar 23, 2017
@Wandalen
Copy link
Contributor Author

OK fixing or OK leaving as is? :) I would be glad to make a pull request.

@addaleax
Copy link
Member

@Wandalen If you want, you can open a PR; the code you’ll want to look at is in setupGlobalConsole() in lib/internal/bootstrap_node.js.

It’s not as simple as just switching a boolean, though; I think you might need to do something like addBuiltinLibsToObject() in lib/internal/module.js to work around the fact that we currently need a getter mechanism for global.console.

I think it’s both ok to fix this and to leave it as is – the property is still configurable, so if you really want to, you can delete global.console; global.console = …; already.

Let us know if you have any questions or there’s something we can help with!

@Wandalen
Copy link
Contributor Author

Yes, sure, I will do. Thank you for hints, Addaleax.

@watilde
Copy link
Member

watilde commented Mar 27, 2017

FYI: I think you could pick a test from WPT to test writable console as like test-whatwg-url-constructor.js.

@Wandalen
Copy link
Contributor Author

Understood, thank you. Will find free time soon.

@Wandalen
Copy link
Contributor Author

Thank you @watilde for the link on w3c test and test from the repo, it was extremely useful. @addaleax you are right, overriding of a console with 2 steps( delete, assign ) was possible. Nevertheless, it was not consistent with the standard.

@Wandalen
Copy link
Contributor Author

The first commit makes console overridable without delete with minimum changes.
The second commit makes console writable, not enumerable and give property descriptor value field as it supposes to be according to the standard.
One more test case stays commented out.
A prototype of a console should have own none fields according to the standard, but currently, have:
'constructor',
'log',
'info',
'warn',
'error',
'dir',
'time',
'timeEnd',
'trace',
'assert'
Please let me know should I make corresponding adjustments to make the test case pass.

@TimothyGu
Copy link
Member

A prototype of a console should have own none fields according to the standard

That is true, but Node.js has always had console as an instance of require('console').Console, and it might be unfortunately too late to change that now due to backwards compatibility requirements.

@Wandalen
Copy link
Contributor Author

@TimothyGu changes which you requested break test suite.

@Wandalen
Copy link
Contributor Author

Wandalen commented Apr 17, 2017

@TimothyGu thanks for your suggestions. Regarding the test case, I can try. Do you think that aspect of functionality covered by tests good enough to trust them? Currently, don't see why it would be impossible.

@TimothyGu
Copy link
Member

@Wandalen let's talk more about what you have included in the PR in the PR itself.

@Wandalen
Copy link
Contributor Author

@TimothyGu, @watilde, @addaleax, @sam-github, @bnoordhuis, @Fishrock123 do you want any other adjustment? Can I be useful with other related issue?

Trott pushed a commit to Trott/io.js that referenced this issue Aug 16, 2017
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: nodejs#11805
Trott pushed a commit to Trott/io.js that referenced this issue Aug 16, 2017
according to the standard
property descriptor of console
should be writable, have value and be not enumerable
adjust test suite imported from w3c/web-platform-tests( console-is-a-namespace )
one test case still is 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: nodejs#11805
Trott pushed a commit to Trott/io.js that referenced this issue Aug 16, 2017
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: nodejs#11805
@Trott
Copy link
Member

Trott commented Aug 16, 2017

I revived #12454 in the hopes of getting it landed (or, if there are firm objections to the whole concept, closed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants