causaly / zod-validation-error

Wrap zod validation errors in user-friendly readable messages
MIT License
813 stars 11 forks source link

fromError returns "Unknown error" for Javascript errors #376

Closed guillaumebrunerie closed 1 month ago

guillaumebrunerie commented 2 months ago

Calling fromError on some standard Javascript error (for instance a ReferenceError) returns the message Unknown error. I think it should instead realize that the error has a message and use that.

jmike commented 2 months ago

This doesn't seem right.

We are handling standard JS errors correctly as you can see in https://github.com/causaly/zod-validation-error/blob/main/lib/toValidationError.ts#L15-L17

Furthermore, I added a PR to add tests for ReferenceError and TypeError in order to prove this is working correctly. https://github.com/causaly/zod-validation-error/pull/377

Could you share a bit more info on how to replicate this issue?

guillaumebrunerie commented 2 months ago

Indeed, my bad. I think my issue was actually that I had a ReferenceError from a different realm (different window object), so the prototype was different and the instanceof check did not work. Unfortunately there is no standard Error.isError yet, but maybe String(err) could be used instead of "Unknown error"?

jmike commented 2 months ago

Unfortunately there is no standard Error.isError yet, but maybe String(err) could be used instead of "Unknown error"?

Hm, let's investigate this together. These are the checks we are making (I am just copy-pasting from https://github.com/causaly/zod-validation-error/blob/main/lib/toValidationError.ts)...

// check if err is a ZodError
if (isZodErrorLike(err)) {
  return fromZodErrorWithoutRuntimeCheck(err, options);
}

// check if err is an instance of the native JS Error
if (err instanceof Error) {
  return new ValidationError(err.message, { cause: err });
}

// at this point err can be anything
return new ValidationError('Unknown error');

Given that err can be anything (e.g. number, Object, Map) it's neither safe nor useful to convert it to String.

jmike commented 1 month ago

This issue is invalid, as explained above. Closing due to inactivity.