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

Fix type of base argument create error #108

Merged

Conversation

jessta
Copy link
Contributor

@jessta jessta commented Jun 8, 2023

This PR fixes the type of the Base argument to createError.

The previous type was Error which is the type of an Error value, but it's being used is as a constructor for a class that inherits from Error.

This change makes the type of Base a constructor for a class that inherits from Error so it's type matches it's usage and documented examples.

Checklist

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

Please use ErrorConstructor if you really means to be a constructor.

@jessta
Copy link
Contributor Author

jessta commented Jun 20, 2023

Please use ErrorConstructor if you really means to be a constructor.

new () => Error is a subset of ErrorConstructor and provides all the functionality required by createError().
It doesn't seem necessary to also require that these classes are also callable as functions.

I'd like to be able to use this with classes that inherit from Error, which aren't callable as functions like ErrorConstructor requires.
eg.
class MyError extends Error {}

@jsumners
Copy link
Member

I'm having difficulty following the reasoning for this PR, but it sounds like a misunderstanding of Error. The implementation here follows the specification:

https://262.ecma-international.org/13.0/#sec-error-objects

creates and initializes a new Error object when called as a function rather than as a constructor. Thus the function call Error(…) is equivalent to the object creation expression new Error(…) with the same arguments.

@jessta
Copy link
Contributor Author

jessta commented Jun 21, 2023

@jsumners The value Error has the type ErrorConstructor, you can call Error as a function eg. Error("some string") or using new Error("some string") and you''ll get a value of type Error.

The current types defined for createError only allow you to pass a value of type Error as the Base parameter, but the function only works if you pass a value of ErrorConstructor. In fact ,the default value assigned to Base is Error which has the type ErrorConstructor.

This PR makes it possible to call createError('CODE', 'hey %s', 500, Error) from Typescript, without this change you can only call createError('CODE', 'hey %s', 500, Error("some string")) which results in a runtime error.

Without this change it's not possible to call createError() correctly from Typescript.

@jsumners jsumners assigned jsumners and unassigned jsumners Jun 22, 2023
@jsumners
Copy link
Member

ping: @fastify/typescript

@Uzlopak
Copy link
Contributor

Uzlopak commented Jun 26, 2023

The finding is correct, it should not be Error.

We have this issue also in fastify
image

I would prefer ErrorConstructor. If you are using typescript wouldnt you use a class anyway?

@climba03003
Copy link
Member

ErrorConstructor is a proper Error structure for JavaScript.
And it also required to provide the function creation method.

interface ErrorConstructor {
    new(message?: string): Error;
    (message?: string): Error;
    readonly prototype: Error;
}

But, I still in favor of using ErrorConstructor as it is more predictable in the structure and follow the spec.

@fox1t
Copy link
Member

fox1t commented Jun 27, 2023

As @Uzlopak said, this is a TS definition issue. Thanks for finding and fixing it, @jessta!
I agree with @climba03003, and I would prefer ErrorConstructor to be used here since it is more flexible and usable by Object.create(.
@jessta I am trying to understand why you would prefer to have only a strict constructor here.

@jessta jessta force-pushed the fix-type-of-base-argument-create-error branch from 4372c10 to 0c544c4 Compare June 29, 2023 03:09
@jessta
Copy link
Contributor Author

jessta commented Jun 29, 2023

I've updated the types to be ErrorConstructor since that seems to be the consensus.

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@Uzlopak Uzlopak requested a review from climba03003 June 29, 2023 05:45
@climba03003 climba03003 merged commit 5b4f5df into fastify:master Jun 29, 2023
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.

5 participants