ResponseValidationError considered to be unsafe?

[Nedgeva]

After getting some run-time exceptions in one app i took a closer look at ResponseValidationError implementation. Things that wonders me:

1. why you're setting prototype of ResponseValidationError class to itself via setPrototypeOf? Isn't it's better to use Symbol.hasInstance for hooking instanceof?
2. ResponseValidationError isn't safe because it's cannot be safely stringifed. Is this intended?

For example following code would produce run-time exceptions:

const failedValidation = pipe(
  fromEither(SomeCoded.decode(someInvalidData)),
  mapLeft<Errors, Error>(ResponseValidationError.create),
);
<RenderRemoteData data={failedValidation} ... />

where RenderRemoteData is something like

interface RenderRemoteDataProps<L, A> {
  data: RemoteData<L, A>;
  failure?: (e: L) => ReactNode;
  ...blah-blah-blah
}

const failureDefault = (e: Error) => (<div>{e.name}: {e.message}</div>);

const Component = <L, A>({
  data,
  failure = flow(toError, /* <-- downcasting ResponseValidationError to Error would cause exception like Uncaught TypeError: Function.prototype.toString requires that 'this' be a Function */ failureDefault),
}: RenderRemoteDataProps<L, A>) => {
  const error = useMemo(() => (e: L) => {
    console.warn('[RemoteDataFailure]', e);
    return failure(e);
  }, [failure]);

  return (
    {isFailure(data) && error(data.error)}
    <Blah />
  );
}

Would like to hear any thoughts on this.

[raveclassic]

why you're setting prototype of ResponseValidationError class to itself via setPrototypeOf?

Because we want instanceof Error to hold for instances of ResponseValidationError.

Isn't it's better to use Symbol.hasInstance for hooking instanceof?

Not sure why is it better? Could you clarify?

ResponseValidationError isn't safe because it's cannot be safely stringifed. Is this intended?

This is likely a bug. Do you mean that we also need to override Object.prototype.toString?

[Nedgeva]

Because we want instanceof Error to hold for instances of ResponseValidationError.

Ok, but seems like it's breaks instanceof checking against parent class ResponseValidationError too.
Consider following code (nevermind light differencies):

class ResponseValidationError extends Error {
  static create(errors: string[]): ResponseValidationError {
    return new ResponseValidationError(errors);
  }

  constructor(readonly errors: string[]) {
    super(errors.join("\\n\\n"));
    this.name = "ResponseValidationError";
    Object.setPrototypeOf(this, ResponseValidationError);
  }
}

const responseValidationError = ResponseValidationError.create(['foo', 'bar']);
console.log(responseValidationError instanceof Error); // expected: false, got: false
console.log(responseValidationError instanceof ResponseValidationError); // expected: true, got: false

Not sure why is it better? Could you clarify?

At least this approach gives (i think) intentional behavior, when responseValidationError is instance only of parent class, not of Error or something.

This is likely a bug. Do you mean that we also need to override Object.prototype.toString?

Yep.

[raveclassic]

but seems like it's breaks instanceof checking against parent class

That's seems like a bug too. We should use the following: https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work

not of Error or something

But it's intentional! :)

[Nedgeva]

Ok then)
If have some time please take a look on suggested solution in PR #94
It aims to fix incorrect instanceof behavior on ResponseValidationError and brings toString method.
Thanks!

[Nedgeva]

But it's intentional! :)

Ouch, seems like i didn't got it at first time. So it was intended that instance of ResponseValidationError will match both ResponseValidationError and Error classes.
Reverted back some changes, only fixed Object.setPrototypeOf(this, ResponseValidationError) to Object.setPrototypeOf(this, ResponseValidationError.prototype).
(But i still think that hooking hasInstance better than manually change prototype chain 🤣 )