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 console.countReset? #89

Closed
domenic opened this issue Mar 3, 2017 · 13 comments
Closed

Add console.countReset? #89

domenic opened this issue Mar 3, 2017 · 13 comments
Assignees
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest

Comments

@domenic
Copy link
Member

domenic commented Mar 3, 2017

Edge has a method console.countReset(label) which allows resetting a counter. This seems useful, and in general it's better to add console methods than remove them, in my opinion.

If we spec this we should make sure that its behavior with no arguments, or undefined, is the same as that is decided for console.count(). (For that, see #88.)

@domfarolino domfarolino self-assigned this Mar 13, 2017
@domfarolino
Copy link
Member

I'm assuming we need some more interest since Edge is the only implementer of this if I'm not mistaken? @JosephPecoraro what do you think about possibly implementing this?

And as far as behavior goes, what should console.countReset() do?

  • Clear the default label
  • Clear all labels

I can see either being valid, but I lean toward the first?

@domenic domenic added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Mar 18, 2017
@jasnell
Copy link

jasnell commented Apr 27, 2017

Just to weigh in here, this would be tremendously useful for Node.js if we were to land the implementation of console.count() there. @domenic has already linked the relevant PR above. The key concern in Node.js would be console.count() becoming the source of memory leaks if we don't have the ability to reset the counters.

@jasnell
Copy link

jasnell commented Apr 27, 2017

@domfarolino ... in edge, console.countReset() only resets the default label so that's likely how it should be defined in the standard. I can see an argument for adding either a console.countResetAll() or even a console.countReset(console.AllCounters) where console.AllCounters is a Symbol but that's purely speculative and of marginal value.

@alexkozy
Copy link

FYI: in current Chrome implementation, console.clear clears all stored messages from internal storage including any counters or timers.

@domenic
Copy link
Member Author

domenic commented Apr 27, 2017

Oh interesting; we should investigate if other browsers do that, and if so, include it in the spec.

@jasnell
Copy link

jasnell commented Apr 27, 2017

@ak239 ... is that a recent fix in Chrome? I just tested locally and as of 58.0.3029.81, calling console.clear() has no effect on the console.count() counters.

@alexkozy
Copy link

@jasnell please check Canary. I think it was regressed a long time ago and we just fixed it back, we can revert this change but it looks good to me.

@jasnell
Copy link

jasnell commented Apr 27, 2017

Ok, if the spec is updated to have console.clear() clear the counters and timers, I will make sure to add that the Node.js impl. That said, I still believe console.countReset() is still a good idea.

@bakulf
Copy link

bakulf commented May 16, 2017

Firefox doesn't cancel timers nor counts. To me console.clear() should just about cleaning console messages. I prefer console.countReset(optional DOMString label = "default");

@fmartin5
Copy link
Contributor

Node.js does implement console.countReset([label='default']) as of v8.3.

@domfarolino
Copy link
Member

Thanks @fmartin5. Looks like we should spec it then, I'm in favor of this, with default behavior being the same as console.count()/console.count(undefined) etc. I can file browser bugs for this soon.

I think the issue of console.clear() clearing other data (like timers and counts) should be a separate issue.

@domfarolino
Copy link
Member

Thanks all! The standard has now been updated with this method :)

@nchevobbe
Copy link

For the record, this is now implemented in Firefox 62.0a1 (2018-05-25)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest
Development

No branches or pull requests

7 participants