console: add dirxml method #17152

Tiriel · 2017-11-20T16:26:46Z

This is an absolute ~~first~~ second draft.
This method was previously exposed by V8 (since Node v8.0.0) and not
implemented in Node directly.
Tests coming soon.

Refs: #17128

Please feel free to comment and give any advice!

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

cjihrig · 2017-11-20T17:42:00Z

lib/console.js

@@ -162,6 +162,25 @@ Console.prototype.dir = function dir(object, options) {
 };


+Console.prototype.dirxml = function dirxml(...data) {
+  const optionProps = ['showHidden', 'depth', 'colors'],
+    maybeOptions = Object.getOwnPropertyNames(data.slice(-1)),


These should be separate const statements.

cjihrig · 2017-11-20T17:51:30Z

lib/console.js

+Console.prototype.dirxml = function dirxml(...data) {
+  const optionProps = ['showHidden', 'depth', 'colors'],
+    maybeOptions = Object.getOwnPropertyNames(data.slice(-1)),
+    isOption = maybeOptions.some((p) => optionProps.indexOf(p) !== -1);


Is the last argument supposed to be an options object? The spec doesn't seem to call for that, and it makes for a slightly awkward UX, IMO. What if we want to display an object that has one of those magic props?

I don't get why this is here. The spec doesn't allow to specify options for dirxml. The fact that there has to be this ugly code to detect it should indicate that it should not exist in the first place.

That's kind of hard said like that.

Anyway, I thought about the possibility of having the last object having precisely one of those options, but I thought that it would be better to allow passing options if need be, just like console.dir() and that the risk would be quite minimal.
Plus, it feels a bit weird to have dir accept options but take only one object, and dirxml not accepting options but taking a varying number of object, but maybe that's just me.
And it does remove the possibility for custom inspect, and so xml redering.

I'll remove it for now anyway, since it seems to be a bad idea, it's indeed akward.

dir explicitly lists options as something that it accepts in the spec: https://console.spec.whatwg.org/#dir

When working on features which have a spec, decisions should always be informed by the behaviour defined there.

Also, since the console spec is an early draft that's classified as a Living Standard, there's always the opportunity for you to go back and try to suggest improvement at the source. But Node.js shouldn't be the origin for that divergence since it just creates confusion for developers.

BTW, the console.dir spec was in fact amended to account for the possibility of having an options object because of Node.js (see whatwg/console#79). However, I agree with @apapirovski that

Node.js shouldn't be the origin for that divergence since it just creates confusion for developers.

-- not if we can help it anyway, which we couldn't with console.dir w/o breaking backwards compatibility.

Yep, definitely agree and good to have a bit of background on that! Thanks @TimothyGu.

Thanks for the background indeed!

However, I don't know if dir is widely used, but I don't know either if it would be worth it to go and ask if the standard could be adapted.
Any opinion on this?

Still dropping this while the standard is as its current state anyway, it does make sense, thank you all!

The problem in this particular case is that dirxml accepts a rest parameter which always has to be final, it's similar in that to log and most other console functions.

There would need to either be a method to set general settings for all dirxml calls or some options object that is exposed on the console, or some sort of a Symbol (similar to like @@toStringTag) that gets declared on the object that decides the formatting preferences.

Indeed that's a problem, that's what had me attempt (poorly) to parse the last object in the list. I don't mind attempting again if the standard does evolve though. If it evolves and if it's worth it. I don't know if it's the case though.

tniessen · 2017-11-20T18:54:51Z

First of all, thank you, @Tiriel!

I was a bit surprised to see this implemented, the MDN page still says:

This feature is non-standard and is not on a standards track. Do not use it on production sites facing the Web: it will not work for every user. There may also be large incompatibilities between implementations and the behavior may change in the future. [...] Displays an interactive tree of the descendant elements of the specified XML/HTML element. [...] The output is presented as a hierarchical listing of expandable nodes that let you see the contents of child nodes.

If that was still the case, I would generally be -1 on implementing this. Even if it has been included in a WhatWG living standard, we are not required to implement it. I have barely ever seen someone use dirxml(), and Firefox only recently updated its implementation according to WhatWG (older versions simply displayed empty XML tags if the contents were not DOM nodes).

If we are implementing this for the sake of conformity with the WhatWG standard, I am okay with this. Apart from that, I am not a fan of the function itself, especially in a non-interactive environment such as the Node.js console. Secondly, the name is misleading in an environment which does not have a concept of XML / HTML.

The standard is vague, but the main aspect of the function is this:

Let converted be a DOM tree representation of item if possible; otherwise let converted be item with optimally useful formatting applied.

We would basically use the fallback in all cases, which is no different than other console functions.

Tiriel · 2017-11-20T19:26:55Z

@tniessen Thanks for the input!

I am actually trying to implement it for conformity with the living standard indeed, as was (very lightly) discussed #17128

I totally agree with you on the use of the fallback though. I tried adding the possibility to pass a custom inspect function, to return proper xml when needed, but was dismissed for quite good reasons ( #17146 ). It would then in effect be really similar to console.dir().

The problem is, we've seen at least two times (need to find again the issues numbers, but at least #16755 ) people searching for the console methods exposed by V8. After discussing a bit with some people, I thought it would be better to have at least a minimal implementation of our own of these methods, or at least the ones present in the Living Standard.

As said in other comments though, if the standard can be made to change (I quite don't understand why they allow passing options in dir but not in dirxml), I'd be happy to improve my implementation. I'm just a bit afraid to ask for these changes, and agree we probably shouldn't be the ones to ask for them.

Hope this makes sense!

Tiriel · 2017-11-22T21:59:06Z

Hi everyone!

I just reworked the function to simply call console.dir for each argument received, as a minimal implementation following the standard.

As said, this implementation is for the sake of conformity until something better can be found, and to avoid having people trying to call the V8 exposed methods with no results.

Documentation and test updated accordingly.

Hope it's ok for everyone!

tniessen · 2017-11-22T23:19:35Z

doc/api/console.md

+
+This method calls `console.dir()` with default options for each argument it
+receives. See [`console.dir()`][] for more details about said defaults.
+Please note that this method doesn't produce any xml formatting and uses the


Nit: Please use does not instead of the abbreviated doesn't.

Nit: xml → XML. When in doubt, check the specification.

tniessen · 2017-11-22T23:25:26Z

doc/api/console.md

+This method calls `console.dir()` with default options for each argument it
+receives. See [`console.dir()`][] for more details about said defaults.
+Please note that this method doesn't produce any xml formatting and uses the
+default `console.dir()` formatting resolution instead.


Not a native English speaker myself, but resolution sounds strange here IMO (at least I never used it in this context). It is not really necessary anyway as the first sentence already has the same information.

Well, I'll just let the precision on the absence of XML formatting then, just to be sure it's clear for everyone. But I thought resolution was indicated, you have me doubting now 😄

tniessen · 2017-11-22T23:54:46Z

lib/console.js

@@ -162,6 +162,13 @@ Console.prototype.dir = function dir(object, options) {
 };


+Console.prototype.dirxml = function dirxml(...data) {
+  for (const item of data) {
+    Console.prototype.dir.call(this, item);


As far as I know, console functions must not be called without a proper context, so this.dir(item) should work.

I'll just check this and make sure it works! Thanks!

tniessen · 2017-11-23T00:18:54Z

test/parallel/test-console.js

+                   "{ foo: 'bar', inspect: [Function: inspect] }\n");
+assert.strictEqual(strings.shift().includes('foo: { bar: { baz:'), true);
+assert.strictEqual(strings.shift().includes('quux'), true);
+assert.strictEqual(strings.shift().includes('quux: true'), true);


Please do not use assert.strictEqual(..., true). Just use assert(...) (or assert.ok(...)) to test for logic values. You can find the documentation here.

Didn't know, sorry. Thanks!

No problem, thanks for going through our (sometimes a little long) process!

tniessen · 2017-11-23T00:26:01Z

Again, thanks for working on this! 😃

I still can't say I like this function or seeing it implemented / used in non-browser environments, but if we consider the WhatWG standard important enough to go by it^[1], I would like to point out that I personally think the standard suggests to print the items the same way console.log() would print them if they were passed as arguments after preprocessing.

In Firefox 57:

In Chrome 58:

Node.js 8.9 with your patch:

I am not saying one behavior is better than the other or that I would personally prefer either, I am just wondering whether it would be a good idea to stick to conventions here. (The standard is really vague.)

^[1] As long as it contains a big red box saying "TODO: This will need a good algorithm" I would not necessarily do that.

cjihrig

LGTM with nits addressed.

Tiriel · 2017-11-23T15:19:10Z

I have a commit ready to be pushed that adresses the nits in doc, but I am on another computer and need to build node first for testing/linting.

Anyway, the commit I want to push also changes the method from parsing args and using console.dir to simply logging using console.log as suggested by @tniessen . Is there consensus on this evolution or do I leave the method as it is in the last commit pushed?

TimothyGu · 2017-11-23T16:03:43Z

Seems like @tniessen is right about using dir vs. log: the spec calls for "optimally useful formatting", which is equivalent to log while dir uses "generic JavaScript object formatting." @Tiriel your changes sound good to me!

Tiriel · 2017-11-23T19:46:51Z

Here we are! @tniessen @TimothyGu @cjihrig PTAL!

And thank you all!

TimothyGu · 2017-11-23T20:41:47Z

doc/api/console.md

+* `...data` {any}
+
+This method calls `console.dir()` with default options for each argument it
+receives. See [`console.dir()`][] for more details about said defaults.


This will require some changes.

Damn, sorry about that. Fixing right now and squashing.

tniessen · 2017-11-24T05:56:39Z

doc/api/console.md

+added: v8.0.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/17128


Nit: This should be https://github.com/nodejs/node/pull/17152 as far as I can tell.

tniessen · 2017-11-24T06:02:04Z

lib/console.js

@@ -162,6 +162,11 @@ Console.prototype.dir = function dir(object, options) {
 };


+Console.prototype.dirxml = function dirxml(...data) {


Nit: This can be simplified to Console.prototype.dirxml = Console.prototype.log as far as I can tell.

This method was previously exposed by V8 (since Node v8.0.0) and not implemented in Node directly. Tests coming soon. Refs: nodejs#17128

…eceived. Minimal implementation following the Living Standard specs, following reviews. Fixes: nodejs#17128

Nits in documentation, rework dirxml to use console.log, tests. Fixes: nodejs#17128

Tiriel · 2017-11-24T10:21:04Z

Nits adressed! Thanks @tniessen

Branch can be merge, but I'll keep a tab to see if some basic XML formatting can be implemented someday in the inspection functions. I'm quite not satisfied with this method being just another alias for log, although it allows a basic implementation for now.

apapirovski · 2017-11-24T10:28:15Z

Branch can be merge, but I'll keep a tab to see if some basic XML formatting can be implemented someday in the inspection functions.

I feel like the naming might be throwing you off. dirxml is supposed to provide a DOM tree representation of a DOM object, it shouldn't really do anything with XML. Since Node doesn't have any sort of a concept of a DOM, there won't ever be need for it to do more than it does currently.

tniessen · 2017-11-24T10:37:25Z

What @apapirovski described is true. The function was designed for browsers, that's why the WhatWG living standard focuses on DOM / XML here, but there really is no concept of that within node. Not even browsers format non-DOM nodes as XML:

Tiriel · 2017-11-24T10:39:21Z

@apapirovski @tniessen I admit at first I have been quite confused and thought it should parse XML, but that's not what I'm talking about now.

The Standard isn't very clear, but it seems to indicate that dirxml should try and convert arguments it receives to display them as XML:

For each item of data:

Let converted be a DOM tree representation of item if possible; otherwise let converted be item with optimally useful formatting applied.

The term "converted" at least seems to indicate that to me. I know Node doesn't implement DOM, so it would be some "display only" thing. And I know browsers don't do that either, but as said, the standard isn't very clear and there are know differences between the standard and the browsers implementations (see the discussion about console.debug).

Anyway as said, it can be merged in this state, it does fulfill the "otherwise" condition above, it's just something I want to investigate on my own afterwards ^^

Thanks again for all the inputs!

tniessen · 2017-11-24T17:11:14Z

but it seems to indicate that dirxml should try and convert arguments it receives to display them as XML

I am not an expert here, but as far as I know, DOM is basically an interpretation of HTML/XML documents, and not every XML document / tree produces a DOM tree (even though every DOM tree can be represented as an XML document). So even if you formatted the data as XML, that still would not make it a "DOM tree representation". I don't think browsers will attempt to display data as DOM trees unless they are DOM nodes. Why would anyone want to have a DOM representation of data which is not DOM?

Anyway, feel free to research this yourself! :)

evanlucas · 2017-11-24T19:16:34Z

I'm not sure it makes sense to add this to console, especially if it doesn't output in XML. Although I greatly appreciate the time you took to work on this @Tiriel I think I'm -1.

devsnek · 2017-11-24T20:02:30Z

in chrome and firefox, as far as i can tell, dirxml is just a direct alias to log. perhaps we should just do the same here.

Tiriel · 2017-11-24T20:05:09Z

@devsnek Well, I don't know how exactly they implement it, but that's the way it is reduced in this PR, and I think that should be the strict minimum implemented, if only to expose it on our own and respect the standard.

I'd like it to present a version of DOM treeish printing, even if it can't be interactive like in the browsers for obvious reasons, but I'll settle for the console.log alias.

apapirovski · 2017-11-24T20:10:39Z

Node core has no concept of DOM... We can't print a DOM tree when we don't know what a DOM node is and isn't. Browsers have specific APIs around the DOM that are not in Node and that are instead re-created by packages like jsdom.

Countless Node apps may receive datas from API dealing XML, or parse DOM documents for various reasons (modifying templates before sending them, or else)

Node apps do not deal with DOM unless they implement jsdom or similar. They're usually dealing either with Virtual DOM (React & similar), or they're dealing with strings that represent HTML/XML.

Node is used for Front apps, at least as much as in backend.

Node is not used for front-end apps. It's used for front-end tooling, which again has no concept of the DOM unless it implements jsdom or similar.

That we don't want to implement the DOM, I can understand, I can only imagine the tremendous amount of work it would require, looking at the packages on npm like jsdom. But we could at least display DOM elements as a pseudo tree, it would make debuging much easier.

See above. We can't display something we don't have any concept of.

To elaborate, most of the core JS implementation in Node.js comes from V8 with certain other APIs implemented by Node instead (setTimeout, TextDecoder, etc.). The DOM (and, more specifically, its implementation) is a wholly separate thing that, within Chrome, is connected to V8 via bindings and that Node.js does not currently have access to nor any hope of having access to.

Tiriel · 2017-11-24T20:21:20Z

@apapirovski Even if that's the case - and I know it is, you're perfectly right - that's still a problem I'll deal with when searching possible solutions. But that's not the purpose of this PR anymore since console.dirxml is presented as a console.logalias.

I've noted that at least one person -1.
I'd like a formal dismissal if the majority thinks it is dumb to merge this PR. I've laid down my arguments for the implementation of a treeish view in this comment #17152 (comment) , but they are quite the same for the alias implementation. Ithink this is quite a minor change that would still allow for a slightly more user friendly experience.

apapirovski · 2017-11-24T20:24:45Z

I'm trying to save you time by explaining why there are not "possible solutions" to the problem you think that there is. This either lands as a simple alias or it won't land at all, there's no better implementation that Node needs or — more importantly — can possibly have. I'm simply trying to avoid a situation where you spend time writing an XML parser or Object to tree formatter, only to have that PR be rejected.

Tiriel · 2017-11-24T20:26:46Z

@apapirovski I do appreciate that, thanks sincerely. But everyone needs a hobby ;)
And in any case, that won't be done in this PR, I intend on leaving it like that. Exactly like you said, it either lands is it is now, or doesn't land at all.

And while I'm at it, I hope I offended no one, it was not my intent. I'm not a native english speaker, I make mistakes, but I do try my best to express myself correctly.

apapirovski

For the record, I think this should land as is. There's no possibility of getting something better and this can potentially allow code re-use in some bizarre code bases that rely on this and don't use the DOM...

tniessen · 2017-11-25T11:09:23Z

It's what I have been saying all along, this function does not make sense in Node.js core, and the only reason to implement it is conformity with the WhatWG living standard. Personally, I don't deem conformity with a half-baked living standard important, but I also won't block this because apparently, others do. I think it won't get any better than this implementation and it does not make things worse than they currently are, except that implementing and documenting it means that people will eventually start using it, and that's when the trouble starts. At the moment, we are kind of at advantage as the standard is vague enough to allow dirxml to be an alias for log.

@evanlucas Please make your -1 explicit if you feel like this should not land.

I'd suggest to keep this open for a couple more days either way so we get a chance to get more feedback from the team. I think @jasnell worked on the console for some time, maybe he has some insights.

jasnell

I'm fine with this strictly as an alias for console.log.

tniessen · 2017-12-03T21:17:29Z

I would like to land this within the next couple of days. Are you okay with that, @evanlucas?

evanlucas · 2017-12-03T21:47:17Z

Yea I don’t want to block this if others want this in. Thanks!

tniessen · 2017-12-03T22:08:47Z

CI: https://ci.nodejs.org/job/node-test-commit/14541/ ~~https://ci.nodejs.org/job/node-test-pull-request/11862/~~

tniessen · 2017-12-03T23:09:11Z

Landed in c84710d.

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 <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn · 2017-12-20T00:17:50Z

Shouldn't this be semver-minor?

Trott · 2017-12-20T00:32:08Z

Shouldn't this be semver-minor?

@gibfahn Depends on your perspective. Previously, the function was exposed (due to the inspector implementation) but was a no-op. Are we adding functionality or fixing something that's broken?

I could go either way, but think semver-minor is probably better (if for no other reason than the "when in doubt, use the bigger change indicator" maxim).

Tiriel · 2017-12-20T08:40:41Z

I know I'm not TSC, but as the OP, if I may just say a word...
Seeing that the other method implemented with this logic, console.debug, landed in the same v9 semver-minor, and that it's marked as semver-minor for lts (per #17033 (comment) ) I'd say there's a good point in making this one semver-minor too.

gibfahn · 2017-12-20T12:05:13Z

Thanks @Trott and @Tiriel

I know I'm not TSC, but as the OP, if I may just say a word...

My opinion is that the more time you've spent on something the more weight your opinion has. Given the work you've done on this PR, your view is definitely useful. Thanks for replying!

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

cjihrig reviewed Nov 20, 2017

View reviewed changes

Tiriel force-pushed the console-dirxml-draft branch from e362fe5 to a69828a Compare November 22, 2017 21:56

Tiriel force-pushed the console-dirxml-draft branch 2 times, most recently from e5d5898 to 4349856 Compare November 22, 2017 22:05

TimothyGu approved these changes Nov 22, 2017

View reviewed changes

tniessen reviewed Nov 22, 2017

View reviewed changes

tniessen reviewed Nov 23, 2017

View reviewed changes

cjihrig approved these changes Nov 23, 2017

View reviewed changes

Tiriel force-pushed the console-dirxml-draft branch from 4349856 to 335896d Compare November 23, 2017 19:45

TimothyGu reviewed Nov 23, 2017

View reviewed changes

Tiriel force-pushed the console-dirxml-draft branch from 335896d to 68d62dd Compare November 23, 2017 20:51

tniessen approved these changes Nov 24, 2017

View reviewed changes

Tiriel added 3 commits November 24, 2017 10:25

console: add dirxml method

52fefd4

This method was previously exposed by V8 (since Node v8.0.0) and not implemented in Node directly. Tests coming soon. Refs: nodejs#17128

console: rework console.dirxml to call console.dir on each argument r…

6319499

…eceived. Minimal implementation following the Living Standard specs, following reviews. Fixes: nodejs#17128

console, doc, test: corrections following review

a8ea202

Nits in documentation, rework dirxml to use console.log, tests. Fixes: nodejs#17128

Tiriel force-pushed the console-dirxml-draft branch from 68d62dd to a8ea202 Compare November 24, 2017 09:26

apapirovski approved these changes Nov 24, 2017

View reviewed changes

Trott approved these changes Nov 25, 2017

View reviewed changes

jasnell approved these changes Nov 25, 2017

View reviewed changes

tniessen self-assigned this Dec 3, 2017

tniessen closed this Dec 3, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added semver-minor PRs that contain new features and should be released in the next minor version. lts-watch-v6.x labels Dec 20, 2017

apapirovski mentioned this pull request Apr 22, 2018

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

Closed

		@@ -162,6 +162,11 @@ Console.prototype.dir = function dir(object, options) {
		};


		Console.prototype.dirxml = function dirxml(...data) {

console: add dirxml method #17152

console: add dirxml method #17152

Conversation

Tiriel commented Nov 20, 2017 • edited Loading

Checklist

Affected core subsystem(s)

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

apapirovski Nov 20, 2017 • edited Loading

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

tniessen commented Nov 20, 2017

Tiriel commented Nov 20, 2017

Tiriel commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tiriel Nov 23, 2017 • edited Loading

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

tniessen commented Nov 23, 2017

cjihrig left a comment

Choose a reason for hiding this comment

Tiriel commented Nov 23, 2017 • edited Loading

TimothyGu commented Nov 23, 2017

Tiriel commented Nov 23, 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

Tiriel commented Nov 24, 2017

apapirovski commented Nov 24, 2017

tniessen commented Nov 24, 2017

Tiriel commented Nov 24, 2017

tniessen commented Nov 24, 2017

evanlucas commented Nov 24, 2017

devsnek commented Nov 24, 2017 • edited Loading

Tiriel commented Nov 24, 2017

apapirovski commented Nov 24, 2017 • edited Loading

Tiriel commented Nov 24, 2017 • edited Loading

apapirovski commented Nov 24, 2017 • edited Loading

Tiriel commented Nov 24, 2017 • edited Loading

apapirovski left a comment

Choose a reason for hiding this comment

tniessen commented Nov 25, 2017

jasnell left a comment

Choose a reason for hiding this comment

tniessen commented Dec 3, 2017

evanlucas commented Dec 3, 2017

tniessen commented Dec 3, 2017 • edited Loading

tniessen commented Dec 3, 2017

gibfahn commented Dec 20, 2017

Trott commented Dec 20, 2017

Tiriel commented Dec 20, 2017

gibfahn commented Dec 20, 2017

Tiriel commented Nov 20, 2017 •

edited

Loading

apapirovski Nov 20, 2017 •

edited

Loading

Tiriel Nov 23, 2017 •

edited

Loading

Tiriel commented Nov 23, 2017 •

edited

Loading

devsnek commented Nov 24, 2017 •

edited

Loading

apapirovski commented Nov 24, 2017 •

edited

Loading

Tiriel commented Nov 24, 2017 •

edited

Loading

apapirovski commented Nov 24, 2017 •

edited

Loading

Tiriel commented Nov 24, 2017 •

edited

Loading

tniessen commented Dec 3, 2017 •

edited

Loading