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

util: change inspect output for boxed primitives #27229

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@BridgeAR
Copy link
Member

BridgeAR commented Apr 14, 2019

This unifies the output to Object(primitive) instead of the former
output. This is less verbose and it should be clearer what the logged
output represents.

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
util: change inspect output for boxed primitives
This unifies the output to `Object(primitive)` instead of the former
output. This is less verbose and it should be clearer what the logged
output represents.
@nodejs-github-bot

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Apr 14, 2019

This is less verbose and it should be clearer what the logged output represents.

It no longer explicitly mentions the type, which I would expect to be confusing? We do print the constructor name for all other objects

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Apr 14, 2019

@addaleax

It no longer explicitly mentions the type

That is correct, but it should still be clear due to the value inside Object(value). It will always represent one kind of primitive. This is also the only way how symbol and bigint values can be wrapped.

We do print the constructor name for all other objects

For other objects it's important to log the constructor name to distinguish them from each other. In this case it's all about the value, not the object.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Apr 14, 2019

In this case it's all about the value, not the object.

@BridgeAR For something like Object(...), I’d expect the prototype to be Object.prototype, not Number.prototype or similar. I know that it’s unambiguous the way it’s printed here, but I feel like we’re loosing specificity without needing to do so?

I do like that this PR makes it easier to figure out how to reconstruct the value from the inspect output.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Apr 14, 2019

For something like Object(...), I’d expect the prototype to be Object.prototype, not Number.prototype or similar.

Yeah, makes sense. I guess there is no ideal way to log these things. I have no strong opinion on this change but I would like to get feedback from others. I personally just do not like the old representation, since it's not clear what it really stands for (I just know that we log boxed primitives like this but if I would just look at a log without a clue about the output, I would not know what it stands for).

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Apr 14, 2019

I know it’s a bit unusual, but how about new Number(42) or new String('foo'), plus Object(BigInt(4n)) and Object(Symbol(foo)) for the two non-constructor variants? That contains both an explicit type, and tells people what these objects really are.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Apr 15, 2019

@addaleax we could do that but it would be more verbose than the current version (it would however improve the readability and remove the ambiguity). There was a recent discussion about not having to verbose descriptions.

@ljharb if I remember correct https://github.com/substack/object-inspect uses the short notation as implemented here. Do you know of any positive / negative feedback about it?

In general: should we try to render more things as it would also be written as input? I thought about using the shorthand notation for functions ({ a() {} } instead of { a: [Function] }). But that might be too implicit? Especially, since we can not tell anything about the argument or the content (while it's possible to read the length of the function but arguments, default parameters or the spread notation could be used).

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Apr 15, 2019

I like the current behaviour more. I think it's more important to accurately represent the current shape of something rather than how we speculate it got to that current shape.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Apr 15, 2019

I’ve had no complaints; and i think this is the cleanest way to represent boxed primitives (acing authored that behavior in object-inspect). It gets tricky if it’s a boxed primitive with properties, but displaying its essence (based on its internal slots) is most important.

@ljharb

ljharb approved these changes Apr 15, 2019

Copy link
Contributor

ljharb left a comment

LGTM but it’d be good to add test cases for boxed primitives with additional properties, and also with an altered prototype chain, and both.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Apr 15, 2019

@devsnek

I like the current behaviour more.

Do you like the current also over the suggestion from @addaleax?

I think it's more important to accurately represent the current shape of something rather than how we speculate it got to that current shape.

I guess with speculate you mean it's about either using Object(5), Object(Number(5)) or new Number(5), new Number('5.0'), etc.?


As I outlined above: I have no strong opinion on this and it was mainly an attempt to be less verbose and to be more clear about it (at least that's how it felt to me).

I'll keep it open a bit longer to get some further suggestions for alternatives and close it otherwise.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Apr 15, 2019

@BridgeAR also the shorthand notation for functions has observable runtime differences; please don’t use it unless the method is actually concise

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Apr 15, 2019

@ljharb that was the reason why I decided against it.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Apr 21, 2019

Since this has no high priority and there is no solution that seems to please everyone it's probably best to stick what we have. Boxed primitives are used rarely and thus it's likely not an issue for most people anyway.

@BridgeAR BridgeAR closed this Apr 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.