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: Add common aliases for console methods #3486

Closed
wants to merge 1 commit into from

Conversation

aks-
Copy link
Member

@aks- aks- commented Oct 22, 2015

@Fishrock123 Fishrock123 added the console Issues and PRs related to the console subsystem. label Oct 22, 2015
@@ -48,6 +48,10 @@ is used on each argument. See [util.format()][] for more information.

Same as `console.log`.

### console.debug([data][, ...])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using the same signature as in the spec: console.debug(object [,object, ...]).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we might want to change the complete doc for console. Because the signature to methods like log are also not matching with spec we are following https://github.com/nodejs/node/blob/master/doc/api/console.markdown#consolelogdata- Do you think I should change them as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think changing the signatures to match the spec would be preferred. Can you do so in a second commit?

@silverwind
Copy link
Contributor

Please also add some tests for these methods here.

@silverwind silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 22, 2015
@jasnell
Copy link
Member

jasnell commented Oct 22, 2015

Wouldn't this need to be semver-major as it could break anyone who has monkey patched these? (same issue as nooping the undefined ones)

@silverwind
Copy link
Contributor

I don't think we have to support broken polyfills or monkey patches, and I think it's unlikely anyone would do so in the case of these methods.

@aredridel
Copy link
Contributor

Hm. on the converse side, this lets things designed for the browser run as is, even if they use these features.

@chrisdickinson
Copy link
Contributor

@jasnell:

Wouldn't this need to be semver-major as it could break anyone who has monkey patched these? (same issue as nooping the undefined ones)

It seems low risk. If code is monkeypatching the console object, that code's new debug/exception methods should (continue to) win. I might be missing something though — do you have a link to the problems arising from noop'ing handy?

@aks- aks- force-pushed the add-aliases-to-console branch 2 times, most recently from b26facc to ae36937 Compare October 28, 2015 19:13
@aks-
Copy link
Member Author

aks- commented Oct 29, 2015

@silverwind I have added the tests for this and changed the signture of newly added aliases as per browser spec. Besides that I created a new PR #3584 in which I changed the method signatures which were there previously in doc to follow the browser spec. I haven't changed the signatures of methods which have differnent behaviour in node than in browser (e.g. console.assert). Now I am in confusion if we are actually following the spec?

@silverwind
Copy link
Contributor

@aks- thanks, I'll have a look later.

This PR is LGTM.

@silverwind
Copy link
Contributor

@aks- on second look, what's up with tools/v8-prof/tick-processor-tmp-1221 in this patch?

@aks- aks- force-pushed the add-aliases-to-console branch from ae36937 to 16641f7 Compare October 30, 2015 05:40
@aks-
Copy link
Member Author

aks- commented Oct 30, 2015

@silverwind sorry I added that by accident I guess...removed it now

@silverwind
Copy link
Contributor

@silverwind
Copy link
Contributor

LGTM, will likely land this tomorrow.

@Fishrock123
Copy link
Contributor

I'm not sure this is a good idea actually if we aren't going to follow the rest of the "spec".

@aks- Sorry, my fault for pulling you into this console-spec thing. :/

@silverwind
Copy link
Contributor

@Fishrock123 I think we should implement as much of this "spec" as is feasible. What's wrong with these alias methods?

@Fishrock123
Copy link
Contributor

Nothing particularly but it's more things we need to keep supporting. Seems low-value -- stuff works fine if it follows this part but not another part kind of thing.

@aks-
Copy link
Member Author

aks- commented Nov 4, 2015

@Fishrock123 it's okay, I am actually learning a lot about the structure of node codebase :)

@silverwind
Copy link
Contributor

Guess I'll put this on hold then.

Interestingly, MDN says debug is deprecated and it lists exception only as _exception. I think some more research into browser behaviour is needed.

@Fishrock123
Copy link
Contributor

See #3584 (comment), the situation is generally very silly.

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016
@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Closing due to lack of activity. Seems like there's very little interest in this in general.

@jasnell jasnell closed this Mar 22, 2016
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. semver-minor PRs that contain new features and should be released in the next minor version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants