FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add methods to console as per spec by aks- · Pull Request #3473 · nodejs/node · GitHub

/ node Public

Add methods to console as per spec - #3473

Closed
aks- wants to merge 2 commits into
nodejs:masterfrom
aks-:add-methods-to-console
Closed

Add methods to console as per spec#3473
aks- wants to merge 2 commits into
nodejs:masterfrom
aks-:add-methods-to-console

Conversation

aks- commented Oct 21, 2015

Copy link
Copy Markdown
Member

Fishrock123 added the console Issues and PRs related to the console subsystem. label Oct 21, 2015

Copy link
Copy Markdown
Contributor

Related: #1799 (comment)

cc @silverwind & @yosuke-furukawa

Comment thread lib/console.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Perhaps we can define a noop function once in the file, and then assign them all to it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+1 on defining a single noop function.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+1 to that too

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

aha, changed the assignments to noop

aks- force-pushed the add-methods-to-console branch from cfa5c51 to 51b8ef9 Compare October 21, 2015 20:39
vkurchatkin added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 21, 2015

jasnell commented Oct 21, 2015

Copy link
Copy Markdown
Member

@aks- ... probably the next thing I'd say is that a note should be added to the docs that these are noops and while they are defined, they are not expected to actually do anything.

aks- commented Oct 21, 2015

Copy link
Copy Markdown
Member Author

@jasnell Hmm +1. I will add them tomorrow the first thing

aks- force-pushed the add-methods-to-console branch from 51b8ef9 to 23cba06 Compare October 21, 2015 22:23

Copy link
Copy Markdown
Member

This should probably be semver-major because it's going to break polyfills that do feature detection, e.g.

if (!console.profile) polyfill();

Stubbing methods is also something of a questionable practice IMO. If you're not supporting something, don't implement it. -1 from me although I don't feel really strongly about it.

Copy link
Copy Markdown
Contributor

Strong -1 here, this would break feature detection as Ben mentions. We have #1799 which takes care of the group* methods, which I plan to land eventually. A noop isn't helping here, and these methods should be taken a look on a case-by-case basis, if something isn't feasable to do in a terminal, we should just throw imho.

Copy link
Copy Markdown
Contributor

@silverwind If we aren't going to follow the console spec there is no reason to land features like console.group() though. Then those belong in user-land.

Fishrock123 added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 21, 2015

Copy link
Copy Markdown
Contributor

@Fishrock123 I'm pretty sure the spec does not dictate to "implement" a noop. I'd certainly prefer to have unimplented stuff as undefined, which is also the strategy browsers follow regarding new API.

cjihrig commented Oct 22, 2015

Copy link
Copy Markdown
Contributor

Also -1 for adding a bunch of noops. Seems like it would be confusing for users to call functions and have nothing happen.

Copy link
Copy Markdown
Contributor

-1 from me.

Copy link
Copy Markdown
Contributor

I think we'd be fine with this PR if it's reduced to just adding the aliases.

aks- commented Oct 22, 2015

Copy link
Copy Markdown
Member Author

I have made a PR doing just that then #3486 And we can discuss it there.

Copy link
Copy Markdown
Contributor

Implementations MUST use a proxy implementation to ensure that calling unimplemented methods will never throw.

@silverwind If this doesn't mean implement no-ops, I don't know what it does mean?

See the top of https://github.com/DeveloperToolsWG/console-object/blob/master/api.md

targos commented Oct 22, 2015

Copy link
Copy Markdown
Member

And the message of the commit that introduced this sentence doesn't help much:
DeveloperToolsWG/console-object@cb3d52e

Copy link
Copy Markdown
Contributor

cc @bkardell who appears to have written it.

Copy link
Copy Markdown
Contributor

I don't think it means doing a noop, but aliasing to console.log.

Copy link
Copy Markdown
Contributor

@silverwind not everything logs there, though. I.e. What should console.clear() do in that case?

aks- commented Oct 23, 2015

Copy link
Copy Markdown
Member Author

Also the signature in our doc doesn't follow the the signature given in console spec. We also have to change that?

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

Given the -1's, the lack of activity, and precedence on similar PRs, closing this one.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants


Back | FazBrowse Home | New Git URL