Structured cloning of Error .name

[Gozala]

Structured cloning of native error types (as per #4268) normalizes .name of the Error back to Error unless it's one of the standard error names. This seems to lead to unexpected behavior with derived Error types. For illustration please consider following example:

class TimeoutError extends Error {
  constructor(message) {
    super(message)
    this.name = 'TimeoutError'
  }
}

const structuredClone = (value) => new Promise((resolve) => {
  const channel = new MessageChannel()
  channel.port2.onmessage = event => resolve(event.data)
  channel.port1.postMessage(value)
})

const main = async () => {
  const cloned = await structuredClone(new TimeoutError('Request time out'))
  // In Chrome 80.0.3987.132
  cloned.stack // => "TimeoutError: Request time out..."
  cloned.toString() // => "Error: Request time out"
  cloned.name // => "Error"
}
main()

1. .stack seems to have capturer name at error site, while .toString() reflects a new name. (Although I suppose that might be implementation bug in how chrome copies stack instead of expected behavior)
2. .name end up as Error even though it was a custom Error class with a different name. (This lead to a bug in our code where we fallback to custom serialization if DataCloneError is thrown)

I would like to point out that identifying custom errors by their .name (or custom .code property established by nodejs) is fairly common. Which is why it is very unfortunate that neither works with structured cloning algorithm. Which is why I would like to propose to either:

1. Not normalize .name on error types (or at least on derived error types) which better support common use case.
2. Introduce some custom field that will be structure serialized.

[domenic]

I think this is reasonable. If you subclass the other structured-cloneable types, e.g. Map or Set, we don't preserve your subclass properties. We just normalize back to the subclass.

[Gozala]

I think this is reasonable.

What are you referring to ?

If you subclass the other structured-cloneable types, e.g. Map or Set, we don't preserve your subclass properties.

Indeed. That also had being somewhat painful with regard to nodejsBuffer that subclasses Uint8Array

We just normalize back to the subclass.

I think you meant back to base Map / Set class, no ?

[domenic]

What are you referring to ?

The current behavior.

I think you meant back to base Map / Set class, no ?

Correct.

[Gozala]

@domenic do you think supporting custom error identifiers (other than with a message field) is a reasonable request worth having discussion about ?

I agree with your point that current behavior aligns with the behavior in regards to other types. At the same time it is unclear why changes to message property are reflected and changes to name are not:

const main = async () => {
  const error = await structuredClone(Object.assign(new Error('original'), { message: 'changed', name: 'OtherError' }))
  error.message // => "changed"
  error.name // => "Error"
}

I imagine this had been debated, but I'm unable to find it.

[domenic]

I'd suggest authors package up custom data they want to transfer alongside their Errors, Maps, or Sets into an object, and structured clone that. E.g. { error: error, customData: "foo" }.

Otherwise, I think it would be neat to improve structured cloning and make it extensible, but that's hard problem. You can see some previous attempts at https://github.com/littledan/serializable-objects

[Gozala]

I'd suggest authors package up custom data they want to transfer alongside their Errors, Maps, or Sets into an object, and structured clone that. E.g. { error: error, customData: "foo" }.

Does not that defeat the the purpose of having #4268 ? Here is the slightly modified example you've provided there:

// page.js
const ts = new TransformStream();

ts.writable.write(1);
ts.writable.write(2);
ts.writable.abort(new TimeoutError("Oh no"));

const worker = new Worker("worker.js");

Doing abort ts.writable.abort({ error: new Error('Oh no'), code: "Timeout" }) would lead to non error object being thrown in when worker does await readable.read().

Otherwise, I think it would be neat to improve structured cloning and make it extensible, but that's hard problem. You can see some previous attempts at https://github.com/littledan/serializable-objects

That does seem like a hard problem, but just copying name property the same way as message property is copied now seems fairly simple and would enable existing patterns. What is the reason behind normalizing name property instead of copying it's value ?

[domenic]

Since we cannot preserve subclasses across realms (unless the custom serializability proposal happens), the example you quote would not work as expected even if we did preserve the name. In particular, causing .name to get out of sync with .constructor/.prototype (including .constructor.name and .prototype.name) is much worse than just following the behavior of Set and Map.

[Gozala]

Since we cannot preserve subclasses across realms (unless the custom serializability proposal happens), the example you quote would not work as expected even if we did preserve the name.

It depends what user expects there. If user expects that error instanceof TimeoutError would not work in the worker, than indeed so. However the reason .code was introduced in nodejs is to identify what kind of error happened (to quote an official doc):

The error.code property is a string label that identifies the kind of error. error.code is the most stable way to identify an error.

In that regard if use expectations is error.name === 'TimeoutError' that would actually work if name field was copied.

In particular, causing .name to get out of sync with .constructor/.prototype (including .constructor.name and .prototype.name) is much worse than just following the behavior of Set and Map.

I am sorry but I do not get that argument. I would buy it if name was readonly property on Error instances, but it is not. Nor it is custom property like code. So I don't think it's comparable to Set and Map in this context, neither have such properties. Comparison to .message property appears more valid, only real difference between name and message is that later can be passed as a constructor argument and former can't.

[Gozala]

Just to clarify originally I was proposing that derived error types were treated differently, but since I have recognized @domenic's argument for not doing that. Since I have attempted to shift discussion towards just treating name property same as message is treated regardless of class hierarchy. My argument for this are:

1. That would enable fairly common pattern in JS to be used (only other alternative would be to rely on messages)
2. name is mutable property, in fact other methods even respect it's mutation (so it's unclear why it needs to be normalized)

   e = Object.assign(new Error('boom!'), {name:'Boom'})
   e.toString() // Boom: boom!

   I think no other built-in supported by structured cloning has similar property. Only thing that could come close to new Error().name = newName would be [1, 2, 3, 4].length = 2 and cloned array would reflect mutation and not an initial state set during construction.