console: add console.count() and console.clear() #12678

jasnell · 2017-04-26T19:46:40Z

Both are simple utility functions defined by the WHATWG console spec.

Ref: #12675

/cc @addaleax

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

console

jasnell · 2017-04-26T19:47:20Z

planning to tackle an implementation of the console.group*() functions separately once I figure out a reasonable approach

addaleax

LGTM with a couple of suggestions

addaleax · 2017-04-26T19:49:20Z

test/parallel/test-console-clear.js

+const common = require('../common');
+const assert = require('assert');
+
+const stdout_write = global.process.stdout.write;


Fwiw I think all the global.s here are redundant

addaleax · 2017-04-26T19:49:51Z

test/parallel/test-console-clear.js

+// The sequence for moving the cursor to 0,0 and clearing screen down
+const check =
+  Buffer.from([0x1b, 0x5b, 0x31, 0x3b, 0x31, 0x48, 0x1b, 0x5b, 0x30, 0x4a])
+        .toString();


Just check = '\u001b[1;1H\u001b[0J' should be fine :)

addaleax · 2017-04-26T19:50:24Z

test/parallel/test-console-clear.js

+
+process.on('exit', () => {
+  assert.strictEqual(buf, check);
+});


I think this doesn’t need to be wrapped in .on('exit')

jasnell · 2017-04-26T19:56:54Z

@addaleax ... updated to fix those nits :-)

jasnell · 2017-04-26T20:06:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7694/

vsemozhetbyt · 2017-04-26T20:07:40Z

doc/api/console.md

+Maintains an internal counter specific to `label` and outputs to `stdout` the
+number of times `console.count([label])` has been called.
+
+```


FWIW: in repl.md there were ```js in such cases and I've tried to save this approach to preserve highlighting with something like this.

Thanks! :-)

YurySolovyov · 2017-04-26T20:20:47Z

lib/console.js

@@ -54,6 +56,8 @@ function Console(stdout, stderr, ignoreErrors = true) {
  prop.value = createWriteErrorHandler(stderr);
  Object.defineProperty(this, '_stderrErrorHandler', prop);

+  this[kCounts] = {};


Why not use Map here? Won't that remove the restriction about symbols?
Or at least Object.create(null) to go straight into dictionary mode.

I've updated to Object.create(null)... the symbol restriction is actually there to approximate the behavior implemented by browsers. We can easily support Symbol if we wanted to.

jasnell · 2017-04-26T20:49:10Z

Failures in CI are unrelated.

targos · 2017-04-26T20:52:39Z

doc/api/console.md

+When `stdout` is a TTY, calling `console.clear()` will attempt to clear the
+TTY. When `stdout` is not a TTY, this method does nothing.
+
+### console.count([label])


YAML block is missing here.

O.o ... sigh. lol

targos · 2017-04-26T20:54:18Z

lib/console.js

+};
+
+// Defined by: https://console.spec.whatwg.org/#table
+Console.prototype.table = Console.prototype.log;


Is this addition intentional?

Oh, yeah, just forgot about it. I need to add that to the documentation :-)

Fishrock123 · 2017-04-26T21:32:33Z

We shouldn't implement console.clear(). Not only does not not actually clear (You can actually clear terminals with hotkeys, but I don't think from stdio) but we aren't actually required to implement it by the spec. It doesn't make much sense in this context, ergo make it a no-op.

vsemozhetbyt · 2017-04-26T21:33:05Z

doc/api/console.md

+* `data` {any}
+* `...args` {any}
+
+Alias for `console.log([data[, ...args]]).


Is this an intentional way to designate optional parameters?

This is just the only such case in the doc and it differs from the very console.log([data][, ...args]) fragment, but if it is intentional, it is OK)

oh, I see what you're referring to. No, that was just a misplaced ] on my part :-)

But it still misses a backtick)

That's fixed too :-)

jasnell · 2017-04-26T21:36:52Z

@Fishrock123

does not actually clear

Not sure what you mean here. It clears just fine in the terminal.

Fishrock123 · 2017-04-26T21:39:01Z

Doesn't that just jump down to make it look like there is no output in the screen? I investigated implementing this a while ago (might have been almost a year ago) and it didn't look like there was a way to actually clear it so there was no current terminal history loaded (e.g. no scroll up).

jasnell · 2017-04-26T21:43:17Z

Sure, it leaves the scrollback buffer in place but that seems like a reasonable compromise.

TimothyGu · 2017-04-26T22:06:24Z

lib/console.js

+};
+
+// Defined by: https://console.spec.whatwg.org/#count
+Console.prototype.count = function count(label = kDefaultCount) {


According to the IDL, label is a DOMString with default being 'default'. This means that the implementation should be akin to:

function count(label = 'default') { label = `${label}`; // ... this.log(`${label}: ${counts[label]}`); }

This should also allow the use of Map for this[kCounts].

Yeah, I saw that but looking at the browser implementations, neither Chrome or Firefox treat console.count() and console.count('default') as being equivalent. They implement separate counters.

Fair enough, but I would still be happier if a) this deviation is documented in a comment, and b) this[kCounts] is a Map.

We've filed bugs and are fixing Chrome and Firefox; patches should be landing within the week.

Given that, I've no problem with using default as the label.

@TimothyGu ... I'm curious why you want this[kCounts] to be a map. It would not have an impact on the ability to use Symbol as a key given the toString() restriction that would still exist. Using the dictionary mode object works just as well. Can you elaborate?

TimothyGu · 2017-04-26T22:07:19Z

lib/console.js

+  // get reset. Within the browser, that likely is not so much of a
+  // problem but it could be a source of nasty memory leaks in a
+  // Node.js process. We could add an option for clearing the counters
+  // but it would be non-standard.


That is indeed a problem. Can we file an issue at WHATWG, or supply a console.countReset(label) as an extension?

Opening an issue in the WHATWG repo would be good. I'd be hesitant to add a non-standard method before it was added to the spec.

whatwg/console#89

TimothyGu · 2017-04-26T22:10:56Z

doc/api/console.md

@@ -250,6 +290,15 @@ If formatting elements (e.g. `%d`) are not found in the first string then
 [`util.inspect()`][] is called on each argument and the resulting string
 values are concatenated. See [`util.format()`][] for more information.

+### console.table([data][, ...args])


I'd prefer console.table(tabularData[, properties]) like the IDL specifies.

TimothyGu · 2017-04-26T22:11:06Z

doc/api/console.md

+* `data` {any}
+* `...args` {any}
+
+Alias for `console.log([data][, ...args])`.


This might be better implemented as console.log(tabularData), since the second properties argument is not necessarily directly printed according to spec. Also, the spec limits the number of arguments to two, so rest arguments shouldn't be used anyway.

I'm contemplating just dropping this in this PR, to be honest. Still stewing on it tho.

jasnell · 2017-04-26T22:45:32Z

@TimothyGu ... I've pushed a new commit that removes the console.table() alias.

@Fishrock123 ... I've added a note clarifying that console.clear() implementation may vary across OS and terminal impls.

not-an-aardvark · 2017-04-26T22:58:20Z

doc/api/console.md

+-->
+
+* `label` {any} Any value (other than `Symbol`) that can be coerced into a
+  string.


Would it be clearer to just say label should be a string? Sure, the function will work if it's passed an object, but the output [object Object]: 1 is usually not what a user would want. Additionally it might be confusing that two distinct objects share a label (since they will both be coerced to [object Object]).

This is fine but to match the behavior in browsers coercion will still be happening and two separate objects would still end up sharing a label unless they take steps to avoid it (e.g. by using Symbol.toPrimitive, etc)

lpinca · 2017-04-27T12:23:31Z

test/parallel/test-console-clear.js

+const common = require('../common');
+const assert = require('assert');
+
+const stdout_write = process.stdout.write;


Nit: how about using camel case for the identifier?

lpinca · 2017-04-27T12:25:07Z

test/parallel/test-console-clear.js

+// Fake TTY
+process.stdout.isTTY = true;
+
+process.stdout.write = common.mustCall((string) => buf += string, 2);


Nit: it doesn't harm, but I find common.mustCall() redundant in this case. The assertion below is sufficient imo.

Fishrock123 · 2017-04-27T14:36:56Z

@jasnell I would prefer to not implement it until we can get clarification on the spec end what "clear" means. https://console.spec.whatwg.org/#clear is, unfortunately, not very clear about that.

jasnell · 2017-04-27T14:41:18Z

@domenic ... any opinions on @Fishrock123's concerns around console.clear()?

domenic · 2017-04-27T15:00:04Z

I don't understand them. The spec is pretty clear that you can do whatever is appropriate for your environment, including nothing. In general if something is about UI, it's up to the implementation to do what it thinks is best for its users.

jasnell · 2017-07-31T16:50:07Z

@TimothyGu ... updated! :-)

TimothyGu

LGTM! (again!)

CI: https://ci.nodejs.org/job/node-test-pull-request/9427/

Both are simple utility functions defined by the WHATWG console spec (https://console.spec.whatwg.org/). PR-URL: #12678 Ref: #12675 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

jasnell · 2017-08-02T15:48:36Z

Landed in cc43c8f ...

fyi @addaleax ... this should cherry-pick cleanly into v8.x-staging if you wanted to pull it in to 8.3.0 at all. :-)

Both are simple utility functions defined by the WHATWG console spec (https://console.spec.whatwg.org/). PR-URL: #12678 Ref: #12675 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

gibfahn · 2018-01-15T22:06:21Z

Release team were +1 on backporting to v6.x.

Both are simple utility functions defined by the WHATWG console spec (https://console.spec.whatwg.org/). PR-URL: #12678 Ref: #12675 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

Both are simple utility functions defined by the WHATWG console spec (https://console.spec.whatwg.org/). PR-URL: #12678 Ref: #12675 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

Both are simple utility functions defined by the WHATWG console spec (https://console.spec.whatwg.org/). PR-URL: #12678 Ref: #12675 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) nodejs#12678 * crypto: - expose ECDH class (Bryan English) nodejs#8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) nodejs#10209 - warn on invalid authentication tag length (Tobias Nießen) nodejs#17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) nodejs#16835 * dgram: - added socket.setMulticastInterface() (Will Young) nodejs#7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) nodejs#13005 * lib: - return this from net.Socket.end() (Sam Roberts) nodejs#13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) nodejs#16386 * net: - return this from getConnections() (Sam Roberts) nodejs#13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) nodejs#13784 * repl: - improve require() autocompletion (Alexey Orlenko) nodejs#14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) nodejs#16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) nodejs#12087 - add process.ppid (cjihrig) nodejs#16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) nodejs#12839 * tools, build: - a new macOS installer! (JP Wesselink) nodejs#15179 * url: - WHATWG URL api support (James M Snell) nodejs#7448 * util: - add %i and %f formatting specifiers (Roman Reiss) nodejs#10308 PR-URL: nodejs#18342

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 26, 2017

nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Apr 26, 2017

addaleax approved these changes Apr 26, 2017

View reviewed changes

jasnell force-pushed the console-new branch from a99f9c4 to 4aba866 Compare April 26, 2017 19:56

addaleax approved these changes Apr 26, 2017

View reviewed changes

vsemozhetbyt reviewed Apr 26, 2017

View reviewed changes

jasnell force-pushed the console-new branch from 4aba866 to 5b87c7e Compare April 26, 2017 20:16

YurySolovyov reviewed Apr 26, 2017

View reviewed changes

jasnell force-pushed the console-new branch from 5b87c7e to f631caf Compare April 26, 2017 20:25

targos reviewed Apr 26, 2017

View reviewed changes

jasnell force-pushed the console-new branch from f631caf to be444b9 Compare April 26, 2017 21:27

vsemozhetbyt reviewed Apr 26, 2017

View reviewed changes

jasnell force-pushed the console-new branch from be444b9 to d719a89 Compare April 26, 2017 21:45

TimothyGu reviewed Apr 26, 2017

View reviewed changes

benjamingr approved these changes Apr 26, 2017

View reviewed changes

not-an-aardvark reviewed Apr 26, 2017

View reviewed changes

lpinca reviewed Apr 27, 2017

View reviewed changes

[Squash] Address final nits

60269cd

TimothyGu approved these changes Aug 1, 2017

View reviewed changes

tniessen approved these changes Aug 2, 2017

View reviewed changes

jasnell closed this Aug 2, 2017

akaila mentioned this pull request Oct 19, 2017

util: adding a unit test to cover invalid code check in util.deprecate method #16305

Closed

3 tasks

gibfahn added the lts-watch-v6.x label Jan 15, 2018

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

console: add console.count() and console.clear() #12678

console: add console.count() and console.clear() #12678

Conversation

jasnell commented Apr 26, 2017

Checklist

Affected core subsystem(s)

jasnell commented Apr 26, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 26, 2017

jasnell commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 26, 2017

Fishrock123 commented Apr 26, 2017

jasnell commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Apr 27, 2017

jasnell commented Apr 27, 2017

domenic commented Apr 27, 2017

jasnell commented Jul 31, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

jasnell commented Aug 2, 2017

gibfahn commented Jan 15, 2018

vsemozhetbyt Apr 26, 2017 •

edited

Loading

TimothyGu Apr 26, 2017 •

edited

Loading