tc39 / proposal-is-error

ECMAScript Proposal, specs, and reference implementation for Error.isError
https://tc39.es/proposal-is-error/
MIT License
62 stars 2 forks source link

Error.isError necessary for properly serializing in debug tools #2

Closed tolmasky closed 8 years ago

tolmasky commented 8 years ago

In Tonic we serialize objects and send them over the wire to be rendered on the client. Unfortunately, Errors are the only objects we can't properly handle when creating in a different realm. I've put together this small test case:

https://tonicdev.com/tolmasky/errors-realm

As you can see, the bottom error renders "correctly": the UI knows its an error and gives it special treatment. However, in the top case it can't know it is an error, and thus defaults to object look.

benjamingr commented 8 years ago

We've had this problem in bluebird too where we detect non-errors for debugging purposes.

ljharb commented 8 years ago

Thanks for contributing this use case! @benjamingr if you have concrete examples, that would help a ton :-) (filed as a separate issue or attached onto this one)

domenic commented 8 years ago

This shows nothing about the necessity of Error.isError. You would have better code if you tested e.g. Object.prototype.toString.call(obj) === "[error Error]". Then objects that wanted to opt in to error-like serialization could do so by setting their toStringTag appropriately. Or, since you are inside a single source realm, just use instanceof.

The only way Error.isError is necessary is if there is a use case for unforgably checking if something is generated from an Error object, which is not shown by this example at all. I claim there can be no such example in existence, since Error objects have no special behavior that cannot be produced by plain JavaScript objects.

benjamingr commented 8 years ago

@domenic worth mentioning that you can also override Symbol.hasInstance and keep using instanceof across realms.

@ljharb https://github.com/petkaantonov/bluebird/issues/990 for example - there are a few others.

tolmasky commented 8 years ago

The purpose of the object viewer is to view the true state of the object graph in question, as such we do not want any opt-in behavior, and do in fact want the unforgable behavior. The viewer is meant to tell you whether this is in fact an Error object per the JavaScript spec. If the spec were to be changed to not have Error objects at all then perhaps that would be another solution to this issue (at which point we show them as object ALWAYS), but today as a debug tool there exists information that cannot be properly represented.

I'm not sure what you mean by being inside a single source realm as I created it in a contextify sandbox (but perhaps I simply misunderstood), this is what happens with instanceof: https://tonicdev.com/tolmasky/56d5d383c14e920d0064d060

More importantly though, instanceof can't be used since due to the Symbol.hasInstnace, instanceof may have side effects. It is absolutely crucial that during object serialization we do not affect the state of your running program.

ljharb commented 8 years ago

There's also that despite "stack" not being part of the ES spec, native Error objects are the only objects that have a reliable stack property - that's something that's useful to have unforgeable. @benjamingr, Symbol.hasInstance only works if there's a cross-realm way to identify Error objects (and Object#toString is forgeable due to Symbol.toStringTag)

ljharb commented 8 years ago

@erights: per https://github.com/tc39/ecma262/pull/438#issuecomment-191283410, do you think the future standardization of a stack on Error instances requires them to be unforgably brand-checkable?

erights commented 8 years ago

@ljharb I think it will cause them to be brand-checkable, in that the behavior of the api -- throw on non-errors -- will provide a brand check. OTOH, it may treat non-errors and errors from other realms identically, in which case it is not a brand check in the traditional sense. So I don't yet know.

ljharb commented 8 years ago

Gotcha, thanks.

ljharb commented 8 years ago

Thanks for this use case!

After discussion with the committee, this proposal is now withdrawn in favor of a proposal that standardizes Error stack traces.