Skip to content

Support cause in Error constructor #112

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

Closed
2 tasks done
erfanium opened this issue Aug 7, 2023 · 13 comments
Closed
2 tasks done

Support cause in Error constructor #112

erfanium opened this issue Aug 7, 2023 · 13 comments
Labels
good first issue Good for newcomers

Comments

@erfanium
Copy link

erfanium commented Aug 7, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

it would be great to be able to use cause option in fastify errors as well

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause

Motivation

No response

Example

const jsError = new Error("hey", { cause: new Error("cause") }) // supported by js
const fError = new FASTIFY_ERROR("hey", { cause: new Error("cause") }); // not supported
@erfanium erfanium changed the title Support adding cause in Error constructor Support cause in Error constructor Aug 7, 2023
@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 7, 2023

Maybe when we drop node 14 👍

@mcollina
Copy link
Member

mcollina commented Aug 7, 2023

There is no need to wait, we could implement it ourselves in this repo.

@mcollina mcollina added the good first issue Good for newcomers label Aug 7, 2023
@erfanium
Copy link
Author

erfanium commented Aug 9, 2023

How could we support this without breaking change?
currently, the FastifyError class constructor receives rest arguments. so we can't add new argument for options.

I'm +1 to change the FastifyError class constructor signature and use normal arguments.

@mcollina
Copy link
Member

mcollina commented Aug 9, 2023

Something like:

// function createError (code, message, statusCode = 500, Base = Error, hasCause = false)
const MyErorr =  createError('ABC', 'something', Error, true)
new MyError({ message, args, cause })

Which is backward compatible.

At some point, we would need to drop (!) the positional arguments and use an object.

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 9, 2023

Is it really preferable to pass in an object?

I created once #103 were the formatter would have been generated on creation of the ErrorConstructor. If we never had rest arguments, the serializer would be faster. And tbh. I dont know how the { args } would work. Also new MyError({ message, args, cause }) doesnt make sense, because we have currently no "message" parameter.

@jsumners
Copy link
Member

jsumners commented Aug 9, 2023

Is it really preferable to pass in an object?

In my opinion, all functions should be declared function ({ ...input }) {} going forward. It is effectively a named parameters implementation and the developer experience benefit far outweighs any negligible performance gains.

Also new MyError({ message, args, cause }) doesnt make sense, because we have currently no "message" parameter.

Technically, the signature is MyError([messageParam1], ..., [messageParamX]). The only requirement the MyError constructor carries over from the standard Error constructor is that it can be invoked with or without new. I want to keep that, but I am all for making the parameters an object.

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 9, 2023

Exactly. So it would something like

MyError({args, cause, statusCode}) and so on. But it would make sense then to change the format string to a simple serializer.

E.g.

const createError = require('@fastify/error')
const MyError = createError('ERROR_CODE', 'Hello %entity')
console.log(new MyError({entity: 'world'})) // error.message => 'Hello world'

@jsumners
Copy link
Member

jsumners commented Aug 9, 2023

I don't think so. It's easier all around to have MyError({ formatValues: ['a', 'b', 42] }) or, if someone really wants to re-supply the names, MyError({ formatValues: { a: 'a', b: 'b', answer: 42 }).

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 9, 2023

It feels like the whole aspect of fast error instanciation get lost, when we pass objects and arrays.

@jsumners
Copy link
Member

jsumners commented Aug 9, 2023

Pretty sure that's never been the goal of this module. I believe its purpose is for generating standard errors with error codes.

@mcollina
Copy link
Member

mcollina commented Aug 10, 2023

It feels like the whole aspect of fast error instanciation get lost, when we pass objects and arrays.

There is absolutely nothing fast in creating an error due to the stacktrace requirement. Fastify kinds of requires the use of Errors for certain patterns and maybe we might want to provide a few fast-paths there.

@DanieleFedeli
Copy link
Contributor

DanieleFedeli commented Sep 27, 2023

I will pick this up, I will open a draft PR soon

EDIT: PR open, I will wait your feedback on the draft before proceeding if you don't mind.

@erfanium
Copy link
Author

Thanks guys for implementing this!

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

No branches or pull requests

5 participants