-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Conversation
Merge conflict |
The behavior needs to be documented. |
The behavior has been documented. |
There was a problem hiding this 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'; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 @fregante Looks like we have a wrong type definition for this feature, Not every Error objects would have the same constructor types |
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 |
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?.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