-
Notifications
You must be signed in to change notification settings - Fork 29k
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
util: use @@toStringTag in util.inspect #16956
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM but this needs a test.
@devsnek thank you for the contribution! |
lib/util.js
Outdated
|
||
// try to use @@toStringTag, fall back to constructor name, empty string | ||
let tag = ''; | ||
if (hasOwnProperty(value, Symbol.toStringTag)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@toStringTag
is inherited. E.g. Object.prototype.toString.call(new Date())
returns [object Date]
, even though new Date().hasOwnProperty(Symbol.toStringTag)
is false. IMO the check for @@toStringTag
should be folded into a part of getConstructorOf
and searched at the same time by walking the prototype chain (of course, then getConstructorOf
would have to be modified to return a string instead).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about getIdentifiableTagOf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
I'm pretty sure I would prefer the constructor name to take precedence over @@toStringTag .. the constructor is at least as specific as the tag, plus this works well with subclasses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 The name before the braces should be a class/constructor name, not anything else, for consistency.
@mscdex if you're worried about consistency, would it help to modify how the tag I'd displayed? Eg in braces or sth like that? |
Symbol.toStringTag is used as class/constructor name by the ECMAScript spec itself, for consistency and compatibility with earlier versions of ES through Object.prototype.toString. So should we. |
It would be semver-major, but perhaps there is a way to show both. Given the example class Foo {
constructor() {}
get [Symbol.toStringTag]() { return 'bar' }
}
const f = new Foo();
util.inspect(f) The current result is |
|
I like |
Works for me. |
will do 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, thanks for the work!
lib/internal/util.js
Outdated
@@ -203,6 +203,41 @@ function getConstructorOf(obj) { | |||
return null; | |||
} | |||
|
|||
function getIdentifiableTagOf(obj) { | |||
let namespace = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace
? Why not call it constructor
?
lib/internal/util.js
Outdated
if (desc.value !== undefined) | ||
tag = desc.value; | ||
else if (desc.get !== undefined) | ||
tag = desc.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get()
should be called with the this
object being set to the original value of obj
… right now this
would refer to the descriptor object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and also maybe wrap this in a try { … } catch
, we don’t really know what the getter will be doing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it throws why wouldn't we want that to bubble up to the user?
lib/internal/util.js
Outdated
if (namespace === tag) | ||
return namespace; | ||
|
||
return `${namespace} ${tag ? `[${tag}]` : ''}`.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the .trim()
?
test/parallel/test-util-inspect.js
Outdated
// @@toStringTag | ||
assert.strictEqual(util.inspect({ [Symbol.toStringTag]: 'a' }), | ||
'Object [a] { [Symbol(Symbol.toStringTag)]: \'a\' }'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test like this?
class Foo {
constructor() {
this.foo = 'bar';
}
get [Symbol.toStringTag]() {
return this.foo;
}
}
assert.strictEqual(util.inspect(new Foo()), 'Foo [bar] { foo: \'bar\' }'));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I don’t think this is a good way to write a class, but it should work :)
If we are adding a new syntax, maybe we should explain it in the docs because one might not be able to immediately tell what |
@refack @TimothyGu @mscdex PTAL |
doc/api/util.md
Outdated
@@ -330,6 +330,18 @@ changes: | |||
The `util.inspect()` method returns a string representation of `object` that is | |||
primarily useful for debugging. Additional `options` may be passed that alter | |||
certain aspects of the formatted string. | |||
`util.inspect()` will use the constructor's name and `@@toStringTag` to make an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: blank line before this to start the new paragraph.
doc/api/util.md
Outdated
} | ||
|
||
util.inspect(new Foo); // 'Foo [bar] {}' | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add some detail on what happens if @@toStringTag
and construtor name are not present... e.g.
`util.inspect()` will use the constructor's name and `@@toStringTag` (if present) to make
an identifiable tag for an inspected value:
Then the examples:
class Foo {
get [Symbol.toStringTag]() {
return 'bar';
}
}
util.inspect(new Foo()); // 'Foo [bar] {}`
function foo() {
return new (class {
get [Symbol.toStringTag]() { return 'bar'; }
});
}
util.inspect(foo()); // '[bar] {}`
class Foo {}
util.inspect(new Foo()); // 'Foo {}`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that second one would actually be Object [bar] {}
, i'm not sure that constructor name will ever be absent.
Object.create(null, { [Symbol.toStringTag]: { value: 'hi' } })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> function m() {
... return new (class {})
... }
undefined
> m()
{}
> m().constructor.name
''
>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> new (class {})
{}
> new (class { get [Symbol.toStringTag]() { return 'tst'; } })
Object [tst] {}
not exactly sure whats going on here but it looks like if the class is empty the inheritance from Object
is optimised away?
In any case i think Object.create
is a really good way to show that the inspected value has no constructor
doc/api/util.md
Outdated
|
||
class Bar {} | ||
|
||
const Baz = Object.create(null, { [Symbol.toStringTag]: { value: 'foo' } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Baz/baz since it's not a constructor
doc/api/util.md
Outdated
|
||
const Baz = Object.create(null, { [Symbol.toStringTag]: { value: 'foo' } }); | ||
|
||
util.inspect(new Foo); // 'Foo [bar] {}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to keep the parentheses after the constructor name, here and on the line below.
As far as the current layout/display goes, I guess it's ok for now, definitely much better than the original. |
lib/internal/util.js
Outdated
if (constructor === tag) | ||
return constructor; | ||
|
||
return `${constructor} ${tag ? `[${tag}]` : ''}`.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could instead just branch on tag
to avoid the trim()
. Something like:
if (tag)
return `${constructor} [${tag}]`;
return constructor;
lib/util.js
Outdated
if (ctx.seen.indexOf(value) !== -1) | ||
return ctx.stylize('[Circular]', 'special'); | ||
|
||
if (recurseTimes != null) { | ||
if (recurseTimes < 0) | ||
return ctx.stylize(`[${constructor ? constructor.name : 'Object'}]`, | ||
'special'); | ||
return ctx.stylize(`[${tag.trim() || 'Object'}]`, 'special'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could avoid trim()
here if we saved the original getIdentifiableTagOf()
result. Maybe change the current tag
variable to prefix
or something to differentiate between the two?
P.S. if we flag |
I would agree that console.log doesn't need all this. i just wanted this for repl |
I don’t think letting |
@refack Using Thought: if |
@TimothyGu As I understand it, both @mscdex and me are -1 on that – showing something that may or may not be a constructor name seems to introduce inconsistency without any need for it… |
+1
I was considering this but i think keeping it distinct is the best option so that when debugging people know where the names are coming from. |
I agree with both statements (log ambiguity, and DevTools!=standard), but in terms of risk/reward IMHO this feature is not worth the risk. It's just my gut feeling, as I don't know what kind of code is out there. I'm not strongly opposed to this landing as non-major, but I'd advise against it. |
I think this should be semver-major. If we are going to be able to change the output of a stable api in a minor/patch version, we should explicitly state "Do not rely on the output of this. It can and will change." in the documentation. Or at least something along those lines. Just because we state that the primary purpose of the api is for debugging doesn't mean it's used that way in practice. |
@devsnek Could you put 6cc4b5e in a separate PR? We might need more discussion around that change on its own, and I would really like the original changes in here to get merged. Also, I'm not sure what that last commit does? We can probably just drop it? We don't usually do merge commits, they break CI because that rebases automatically.. |
er its just a regular merge? i can squash my commits if you want, i had to merge it because there was a conflict |
b48d0df
to
a107aaf
Compare
Please rebase instead. |
uses @@toStringTag when creating the "tag" for an inspected value
a107aaf
to
46eef42
Compare
Landed in 31e0dbc |
uses @@toStringTag when creating the "tag" for an inspected value PR-URL: #16956 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Uses
@@toStringTag
beforeconstructor.name
which yields names where there previously weren't or more correct names.closes #16952
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
util