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

RFC: Updating object-stringifying in v8 error messages #17691

Closed
caitp opened this issue Dec 14, 2017 · 3 comments
Closed

RFC: Updating object-stringifying in v8 error messages #17691

caitp opened this issue Dec 14, 2017 · 3 comments
Labels
discuss Issues opened for discussions and feedbacks. v8 engine Issues and PRs related to the V8 dependency.

Comments

@caitp
Copy link
Contributor

caitp commented Dec 14, 2017

Hi Node.js community

I did some work recently in response to a bug filed about Symbol.toStringTag not being used if a class inherited a null prototype (https://chromium-review.googlesource.com/c/v8/v8/+/820292) (of course, in reality Symbol.toStringTag wasn't used at all in the case the bug mentioned, due to being an accessor property, but still!)

This CL would take the "render-constructor-name" path in more (most) cases, whether the stringified object is not a function, and isn't an Error or doesn't have the same toString method as error, and doesn't have a usable Symbol.toStringTag data property.

Making this change results in some other effects, for instance Proxies no longer look like [object Object], but instead tend to render as #Proxy.

I also feel that it would be helpful for developers to report more information for proxies, for instance also providing some information about the target object. However, this is a bit of an information leak, and makes "membranes" a little bit invisible. This isn't done in the current CL, but maybe it's something that would be worth tacking on. Something like #Proxy<MyFancyFrameworkClass> or something?

So, I wanted to get some feedback from Node-core (and Node users) before landing any of these changes upstream. What would you think about making Proxy objects more clearly Proxies? How much pain would the rest of the CL cause in general, with respect to rendering the constructor-derived string (Are people doing weird things like parsing error messages or something?)

@TimothyGu
Copy link
Member

TimothyGu commented Dec 15, 2017

What would you think about making Proxy objects more clearly Proxies?

Node.js core have taken steps to make it more visible due to users' demand in #16485. Doing something like that in V8 would bring more consistency to Node.js, which is always good.

How much pain would the rest of the CL cause in general, with respect to rendering the constructor-derived string

I don't think it would cause much pain at all. In Node.js we are not really using NoSideEffectToString that much (only in very select instances such as Promise rejection deprecation messages). Most of the error messages that use NoSideEffectToString come from V8 anyway through Signature and AccessorSignature checking.

(Are people doing weird things like parsing error messages or something?)

Probably, but I don't think it should be a huge concern, as I doubt people are testing Node.js' deprecation messages or type safety of their native bindings with Proxy objects. Or that people are using Proxy objects much at all.


That was me speaking as a Node.js Collaborator. But as a user of Node.js I'm personally not a fan of exposing more Proxy guts. As a developer of jsdom, we use Proxy to implement certain magical DOM objects such as NamedNodeMap (used for element.attributes). (Side-note: we were featured in one of the V8 blog posts on Proxy performance!) Whereas before #16485 the object is neatly printed as:

NamedNodeMap { '0': Attr {}, '1': Attr {} }

after it it would be shown as

Proxy [ NamedNodeMap {},
  { get: [Function: get],
    has: [Function: has],
    ownKeys: [Function: ownKeys],
    getOwnPropertyDescriptor: [Function: getOwnPropertyDescriptor],
    set: [Function: set],
    defineProperty: [Function: defineProperty],
    deleteProperty: [Function: deleteProperty],
    preventExtensions: [Function: preventExtensions] } ]

Not helpful at all for users of jsdom, who honestly cares more about if an object is a NamedNodeMap than the exact hooks that object has. And this new output is obviously obscuring (please excuse the oxymoron) if the object is a NamedNodeMap.

But then I realized

  1. that I'm just ranting and actively avoiding writing a custom inspection function,
  2. that jsdom is probably one of the only dozen projects that use Proxy objects for a legitimate purpose and wish to be transparent about it, and
  3. that there will be far more people confused about how Proxy works than people who want Proxy to be transparent.

All of this is however tangential to the original CL, since Node.js' REPL output has nothing to do with V8, and Node.js uses NoSideEffectToString too little for it to matter either way.


Hope this helps.

@TimothyGu TimothyGu added discuss Issues opened for discussions and feedbacks. v8 engine Issues and PRs related to the V8 dependency. labels Dec 15, 2017
@caitp
Copy link
Contributor Author

caitp commented Dec 15, 2017

So far it sounds like updating the error message formatting shouldn't hurt things in Node too much, so that's good to hear.

@TimothyGu
Copy link
Member

Let us know if there are any follow-up questions, but I'll be closing this as it seems discussion has ended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

2 participants