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.

Sign up for GitHub

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: add dirxml method #17152

Closed
wants to merge 3 commits into from
+29 −12
Closed

console: add dirxml method #17152

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52fefd4
console: add dirxml method
Tiriel Nov 20, 2017
6319499
console: rework console.dirxml to call console.dir on each argument r…
Tiriel Nov 22, 2017
a8ea202
console, doc, test: corrections following review
Tiriel Nov 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
console: add dirxml method 
This method was previously exposed by V8 (since Node v8.0.0) and not
implemented in Node directly.
Tests coming soon.

Refs: #17128
@Tiriel
Tiriel committed Nov 24, 2017
commit 52fefd42462aec15d7173019c23cb7e7f1c309f8
19 changes: 19 additions & 0 deletions lib/console.js
View file
Original file line number Diff line number Diff line change
@@ -162,6 +162,25 @@ Console.prototype.dir = function dir(object, options) {
};


Console.prototype.dirxml = function dirxml(...data) {
Copy link
Member

Choose a reason for hiding this comment

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

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

const optionProps = ['showHidden', 'depth', 'colors'],
maybeOptions = Object.getOwnPropertyNames(data.slice(-1)),
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be separate const statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted!

isOption = maybeOptions.some((p) => optionProps.indexOf(p) !== -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@apapirovski apapirovski Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

let options = { customInspect: false };

if (isOption) {
options = Object.assign(data.splice(-1), options);
}
for (const item of data) {
write(this._ignoreErrors,
this._stdout,
util.inspect(item, options),
this._stdoutErrorHandler,
this[kGroupIndent]);
}
};


Console.prototype.time = function time(label = 'default') {
// Coerces everything other than Symbol to a string
label = `${label}`;