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

Add methods to console as per spec #3473

Closed
wants to merge 2 commits into from

Conversation

aks-
Copy link
Member

@aks- aks- commented Oct 21, 2015

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

Related: #1799 (comment)

cc @silverwind & @yosuke-furukawa

Console.prototype.timelineEnd = function() {};


Console.prototype.timeStamp = function() {};
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

+1 on defining a single noop function.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to that too

Copy link
Member Author

Choose a reason for hiding this comment

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

aha, changed the assignments to noop

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

jasnell commented Oct 21, 2015

@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-
Copy link
Member Author

aks- commented Oct 21, 2015

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

@aks- aks- force-pushed the add-methods-to-console branch from 51b8ef9 to 23cba06 Compare October 21, 2015 22:23
@bnoordhuis
Copy link
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.

@silverwind
Copy link
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.

@Fishrock123
Copy link
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 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
@silverwind
Copy link
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
Copy link
Contributor

cjihrig commented Oct 22, 2015

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

@evanlucas
Copy link
Contributor

-1 from me.

@silverwind
Copy link
Contributor

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

@aks-
Copy link
Member Author

aks- commented Oct 22, 2015

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

@Fishrock123
Copy link
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
Copy link
Member

targos commented Oct 22, 2015

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

@Fishrock123
Copy link
Contributor

cc @bkardell who appears to have written it.

@silverwind
Copy link
Contributor

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

@Fishrock123
Copy link
Contributor

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

@aks-
Copy link
Member Author

aks- commented Oct 23, 2015

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

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

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

@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-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