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: implement console.table and console.dirxml #17128

Closed
Tiriel opened this issue Nov 18, 2017 · 15 comments
Closed

console: implement console.table and console.dirxml #17128

Tiriel opened this issue Nov 18, 2017 · 15 comments
Labels
console Issues and PRs related to the console subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@Tiriel
Copy link
Contributor

Tiriel commented Nov 18, 2017

Hi everyone!

Following #17004 , #17033 and a discussion with @Trott , I'd like to suggest we implement the remainder of the console methods described in the WHATWG living standard.
Most of them are already implemented, the only ones left are console.table() and console.dirxml().

Unless everyone thinks it's a waste of time, I'd like to give it a shot. But of course, help will be deeply welcomed.
In any case, I think I'll start off with console.dirxml() if it's ok.

Comments and advices welcomed!

@devsnek
Copy link
Member

devsnek commented Nov 18, 2017

if you're doing dirxml i'd be interested in trying out table

@devsnek
Copy link
Member

devsnek commented Nov 18, 2017

currently i tried just using \t to separate columns, resulting in the output below, if anyone has a better idea let me know. We'll also need to use something else besides util.inspect on non-primitive values.

// an array of strings
console.table(["apples", "oranges", "bananas"]);


// an object whose properties are strings

function Person(firstName, lastName) {
  this.firstName = firstName;
  this.lastName = lastName;
}

var me = new Person("John", "Smith");

console.table(me);


// an array of arrays

var people = [["John", "Smith"], ["Jane", "Doe"], ["Emily", "Jones"]];

console.table(people);


// an array of objects, logging only firstName

function Person(firstName, lastName) {
  this.firstName = firstName;
  this.lastName = lastName;
}

var john = new Person("John", "Smith");
var jane = new Person("Jane", "Doe");
var emily = new Person("Emily", "Jones");

console.table([john, jane, emily], ["firstName"]);


var family = {};

family.mother = new Person("Jane", "Smith");
family.father = new Person("John", "Smith");
family.daughter = new Person("Emily", "Smith");

console.table(family);


@mscdex mscdex added console Issues and PRs related to the console subsystem. feature request Issues that request new features to be added to Node.js. labels Nov 18, 2017
@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 19, 2017

Awesome!

IMHO the width of the columns should be controlled though, and maybe the table be defined with border, so to replicate browser implementation at the best.

Thanks for your work! Starting on dirxml today.

@Trott
Copy link
Member

Trott commented Nov 19, 2017

If it matters for any attempts to implement table() (and it probably doesn't, but just in case), our current minimal implementation of console.group() uses two spaces for each level of indentation and not a tab character.

We could have gone with a tab character instead, but didn't in order to be consistent with what the util module does for indentation.

@Trott
Copy link
Member

Trott commented Nov 19, 2017

...although a tab character might make a lot more sense here for simplicity. I think a simple and lightweight implementation for something like console.table() is more desirable (at least initially) than a full-featured implementation. My thinking is:

  • Most people won't use this function at all, so putting a ton of code into core for it probably isn't a good investment.
  • But it is part of the living standard and it's currently exposed as a no-op, so a minimal implementation makes sense.
  • If people want to add a lot of bells and whistles to it, a userland module can override console.table.

@Trott
Copy link
Member

Trott commented Nov 19, 2017

I don't think we want borders or background colors or other things like in the browser. The browser gets to make lots of assumptions about the console environment, but we don't. The more complex it is, the more maintenance problems are likely to come up.

I'm feeling pretty +1 on separating with a tab character and not worrying about column widths or things lining up. If they don't line up, then the end user will need to adjust their tab width. That's unfortunate, but not terrible, at least for a first implementation. If there's a huge outcry for more features, they can come later. (And you now have an easy function for making tsv output to boot!)

If we want things to line up, it will mean getting the length of everything in the table, tracking it, and padding values appropriately. That's do-able, but bug-prone because you have to worry about things like surrogate pairs. I'm feeling really good about "separate with a tab character and let stuff end up wherever it ends up", although I imagine others might be concerned that we'll get bug reports. Maybe just make it clear in the docs that that is all it does.

@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 19, 2017

@Trott Without going full length on replicating browser implementation, I think we should at least get a separation character to get a bit clearer.
But indeed, if it's clearly specified in the docs that this is a minimal implementation, we could drop on other "features".

As for userland modules, I think one already exists. Maybe some inspiration can be found there? Although its implementation does not seem full of bells and whistles either tbh...

@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 19, 2017

Quick question here, for advice.

Since Node.js doesn't deal directly with the browser DOM, it doesn't implement any DOMParser. This will evidently cause some problems to implement console.dirxml(). But is it really necessary?

I mean, the easy way would be to implement dirxml as an alias of console.dir(). But since many APIs throughout the web are still using xml, wouldn't it be better to properly parse it? Do someone think there are a sufficient amount of users willing to properly log xml to justify implementing this method as the WHATWG defines it?

Or am I just going through util.js looking for a way to proxy xml for nothing?

@addaleax
Copy link
Member

I think the best thing Node can do for a situation like that is to provide a symbol that lets you override what console.dirxml() does when it is called with an object that bears that symbol, kind of like util.inspect.custom or util.promisify.custom?

@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 19, 2017

@addaleax In that case, util.inspect.custom itself might do the trick?
Like, default-> alias for console.dir(), with Symbol -> whatever you want?

Or is it so late that I don't understand squat?

@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 19, 2017

Okay, I've worked out an absolute first draft, and soon will open a PR.

I'm currently learning toward providing the possibility to pass a third argument (the first two being the object to inspect and the options object, for consistency with console.dir).
This last argument would either be a custom inspect function or a boolean indicating the object used as first argument has a .inspect() method (but will then be marked as deprecated).
Obviously, if the third argument is omitted or equals false, the method wil act just like console.dir().

Does that sound okay?

Tiriel added a commit to Tiriel/node that referenced this issue Nov 20, 2017
This is an absolute first draft.
This method was previously exposed by V8 (since Node v8.0.0) and not
implemented in Node directly.
Tests coming soon.

Refs: nodejs#17128
@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 20, 2017

First draft: #17146

Tiriel added a commit to Tiriel/node that referenced this issue Nov 24, 2017
This method was previously exposed by V8 (since Node v8.0.0) and not
implemented in Node directly.
Tests coming soon.

Refs: nodejs#17128
Tiriel added a commit to Tiriel/node that referenced this issue Nov 24, 2017
…eceived.

Minimal implementation following the Living Standard specs, following
reviews.

Fixes: nodejs#17128
Tiriel added a commit to Tiriel/node that referenced this issue Nov 24, 2017
Nits in documentation, rework dirxml to use console.log, tests.

Fixes: nodejs#17128
tniessen pushed a commit that referenced this issue Dec 3, 2017
This method was previously exposed by V8 (since node 8.0.0) but not
implemented in node.

PR-URL: #17152
Refs: #17128
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
This method was previously exposed by V8 (since node 8.0.0) but not
implemented in node.

PR-URL: #17152
Refs: #17128
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
This method was previously exposed by V8 (since node 8.0.0) but not
implemented in node.

PR-URL: #17152
Refs: #17128
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@apapirovski
Copy link
Member

These were added in #18137 and #17152

@thw0rted
Copy link
Contributor

Just noticed, the 10.0 docs list console.table() twice, once under the "normal" section and once under the heading for functions that only work with --inspect. I'm assuming that this issue means table is always available, so the second entry in the docs should be removed.

@Tiriel
Copy link
Contributor Author

Tiriel commented Apr 26, 2018

@thw0rted you're perfectly right. I'm pretty sure the method has been implemented the last few days/weeks, but I can't find the PR. The docs may not have been updated completely. There's another PR opened, I'll pass your notice

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. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

7 participants