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

Added support for cause #116

Merged
merged 9 commits into from
Oct 3, 2023
Merged

Added support for cause #116

merged 9 commits into from
Oct 3, 2023

Conversation

DanieleFedeli
Copy link
Contributor

Checklist

This PR target #112. It will be possible to create an error with cause.

Typescript and Documentation changes will be applied after the main idea is approved.

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 27, 2023

I dont think that this is an appropriate implementation. Cause is usually an optional parameter.

@erfanium
Copy link

erfanium commented Sep 27, 2023

new FastifyError(new Error('casue')) syntax doesn't looks good to me. I think it's better to follow actual JavaScript implmentation:

new FastifyError('message', {
  casue: new Error('casue')
})

i'm also okay with this idea, using named arguments

new FastifyError({
  message: 'message',
  casue: new Error('casue')
})

@erfanium
Copy link

erfanium commented Sep 27, 2023

Also hasCause option for createError function is not really needed.
I believe it can be dynamic

@DanieleFedeli
Copy link
Contributor Author

Also hasCause option for createError function is not really needed. I believe it can be dynamic

I am thinking how to make this backward compatible since the function FastifyError accepts a rest arg, maybe it will be needed to change the interface of the function accepting an object instead of positional parameters. Any idea?

@erfanium
Copy link

erfanium commented Sep 27, 2023

@DanieleFedeli I have no idea, but i think it's okay to release a major version if needed

@climba03003
Copy link
Member

I am thinking how to make this backward compatible since the function FastifyError accepts a rest arg, maybe it will be needed to change the interface of the function accepting an object instead of positional parameters. Any idea?

It doesn't really matter for the ...rest.

function Error(...args) {
  if(args.length && typeof args.at(-1) === 'object' && 'cause' in args.at(-1)) {
    this.cause = args.pop()
  }
}

@DanieleFedeli DanieleFedeli marked this pull request as ready for review September 28, 2023 08:25
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@DanieleFedeli
Copy link
Contributor Author

Should I fix for node 14?

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 29, 2023

Yes, please.

index.js Outdated
this.code = code
this.name = 'FastifyError'
this.statusCode = statusCode

if (args.length && typeof args.at(-1) === 'object' && 'cause' in args.at(-1)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (args.length && typeof args.at(-1) === 'object' && 'cause' in args.at(-1)) {
const lastElement = args.length - 1
if (lastElement !== -1 && args[lastElement] && typeof args[lastElement] === 'object' && 'cause' in args[lastElement]) {

I added the args[lastElement] for the case that the last element is null.

Copy link
Contributor

Choose a reason for hiding this comment

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

not tested.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

mcollina commented Oct 3, 2023

@Uzlopak could you do a final check?

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
Copy link
Contributor

Uzlopak commented Oct 3, 2023

@mcollina :)

@mcollina mcollina merged commit ad99cd0 into fastify:master Oct 3, 2023
15 checks passed
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.

6 participants