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

Preserve error constructor #70

Merged
merged 17 commits into from
Apr 18, 2022

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Apr 2, 2022

Previous notes about native-only support

Partially fix for #48.

Includes and is blocked by #69

Custom constructors will need extra logic, because what happens when CustomError’s signature requires a second parameter?

  • Deserialization should catch that and use a regular Error, and/or
  • The user should pass .set(MyError, object => new MyError(object.message, object.otherProp))

But this might require further bikeshedding, so I'm leaving it out of this PR

Fixes #48

@fregante fregante marked this pull request as ready for review April 2, 2022 07:47
@sindresorhus
Copy link
Owner

Merge conflict

@sindresorhus
Copy link
Owner

The behavior needs to be documented.

@fregante
Copy link
Contributor Author

The behavior has been documented.

@fregante fregante changed the title Preserve error constructor (if native) Preserve error constructor Apr 10, 2022
Copy link
Contributor Author

@fregante fregante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be good to go. Do you think this is a breaking change?

  deserializeError({name: 'TypeError'})
- //=> [Error: 🟣] // v9
+ //=> [TypeError: 🟣] // next

@@ -1,5 +1,7 @@
import {Primitive, JsonObject} from 'type-fest';

export {default as errorConstructors} from './error-constructors.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds support for custom error constructors to be added. This only supports simple errors so I mentioned this in the docs and in the types. A new issue can be created to add support for such error constructors.

Also, instead of adding error-constructors.js to the exports map, I just added another export here. It's cleaner anyway.

@@ -46,6 +48,8 @@ const toJSON = from => {
return json;
};

const getErrorConstructor = name => errorConstructors.get(name) || Error;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried ?? but it fails in Node 12. If desired, Node 12 support can probably be dropped separately so it appears in the changelog

@sindresorhus sindresorhus merged commit 49db63a into sindresorhus:main Apr 18, 2022
@sindresorhus
Copy link
Owner

https://github.com/sindresorhus/serialize-error/releases/tag/v10.0.0

@ghost
Copy link

ghost commented May 31, 2022

@sindresorhus @fregante Looks like we have a wrong type definition for this feature,

#78

Not every Error objects would have the same constructor types

@fregante
Copy link
Contributor Author

fregante commented Jun 1, 2022

It seems that constructors like DOMExceptions don't match ErrorConstructor. While therefore the types are incorrect, it doesn't affect the final user. If you have a solution please send a PR, but otherwise I don’t think it's important here.

I think potentially the solution would be to replace ErrorConstructor with new (string, ...anything[]) => {message: string, ...anything} so it matches any constructors that accepts a string and return an ErrorLike

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error subclasses are lost when deserializing
2 participants