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, util: add dirxml method #17146

Closed
wants to merge 1 commit into from

Conversation

Tiriel
Copy link
Contributor

@Tiriel Tiriel commented 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: #17128

Please don't hesitate to leave me an advice. Tests are coming, and doc, but I first want to be sure I'm doing it right, or not bad at least.

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, util

@nodejs-github-bot nodejs-github-bot added console Issues and PRs related to the console subsystem. util Issues and PRs related to the built-in util module. labels 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
@apapirovski
Copy link
Member

apapirovski commented Nov 20, 2017

If we are implementing these then we should be following the spec as far as the arguments available go. https://console.spec.whatwg.org/#dirxml

@apapirovski
Copy link
Member

apapirovski commented Nov 20, 2017

In general, I think there's some confusion here around what dirxml is supposed to do. It takes a list of arguments and for each, formats it either as a DOM tree (if the item can be formatted as a DOM tree and the implementation supports it) or it applies minimal useful JS object formatting.

This means that it is not supposed to parse XML or anything similar. This is one of those methods that's a whole lot more useful in the browser because DOM nodes are just JS objects.

If we are implementing this in Node, then it should be very similar to dir except it can take many arguments. It should not accept any sort of params around options or xml parsing, as the spec does not allow for it.

@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 20, 2017

@apapirovski You make quite a good point, and perhaps I misunderstood the method's purpose indeed. I'll try and rework this as you stated!

Thanks!

@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 20, 2017

In fact, as I'm not touching utils anymore, I'm going to close this PR and open a new one I think.

Thanks again!

@Tiriel Tiriel closed this Nov 20, 2017
@Tiriel Tiriel mentioned this pull request Nov 20, 2017
4 tasks
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. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants