Skip to content

fix: throw hooks promise rejection (#2070)#2074

Merged
mcollina merged 1 commit intofastify:masterfrom
Ravit436:fix/throwHooksPromiseRejection
Feb 3, 2020
Merged

fix: throw hooks promise rejection (#2070)#2074
mcollina merged 1 commit intofastify:masterfrom
Ravit436:fix/throwHooksPromiseRejection

Conversation

@Ravit436
Copy link
Copy Markdown
Contributor

@Ravit436 Ravit436 commented Feb 2, 2020 •
edited
Loading

Fixes: #2070

The reason why the error was not thrown because Promise.reject() was empty and it didn't have any error like Promise.reject(new Error) so when result.then is called it when to handleReject with error undefined.

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

tests-checker[bot]
tests-checker bot suggested changes Feb 2, 2020
View reviewed changes
Copy link
Copy Markdown

@tests-checker tests-checker bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for contributing!
It appears that you have changed the framework code, but the tests that verify your change are missing. Could you please add them?

@Ravit436 Ravit436 mentioned this pull request Feb 2, 2020
Closed
@mcollina
Copy link
Copy Markdown
Member

mcollina commented Feb 2, 2020

Would you mind to use our internal error system? https://github.com/fastify/fastify/blob/master/lib/errors.js it will also need a an error code.

This PR also need tests, and an update to the docs about the new error code.

@Ravit436 Ravit436 force-pushed the fix/throwHooksPromiseRejection branch from 3592375 to 93f140f Compare February 2, 2020 13:40
@allevo
Copy link
Copy Markdown
Member

allevo commented Feb 2, 2020

with this PR, Promise.reject(null), Promise.reject(false), Promise.reject(''), Promise.reject(0), Promise.reject(undefined), Promise.reject() will not supported.
Is this ok?

@Ravit436
Copy link
Copy Markdown
Contributor Author

Ravit436 commented Feb 2, 2020 •
edited
Loading

with this PR, Promise.reject(null), Promise.reject(false), Promise.reject(''), Promise.reject(0), Promise.reject(undefined), Promise.reject() will not supported.
Is this ok?

Now it will support only undefined case. Fixed

@Ravit436 Ravit436 force-pushed the fix/throwHooksPromiseRejection branch from 93f140f to 93610a2 Compare February 2, 2020 15:15
@Ravit436 Ravit436 force-pushed the fix/throwHooksPromiseRejection branch from 93610a2 to 910d3f5 Compare February 2, 2020 15:42
@Ravit436
Copy link
Copy Markdown
Contributor Author

Ravit436 commented Feb 2, 2020

Would you mind to use our internal error system? https://github.com/fastify/fastify/blob/master/lib/errors.js it will also need a an error code.

This PR also need tests, and an update to the docs about the new error code.

Done with the changes, added a new error code.

mcollina
mcollina approved these changes Feb 2, 2020
View reviewed changes
Copy link
Copy Markdown
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

RafaelGSS
RafaelGSS approved these changes Feb 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@Eomm Eomm added bugfix Issue or PR that should land as semver patch v2.x Issue or pr related to Fastify v2 labels Feb 2, 2020
@mcollina mcollina merged commit c052c21 into fastify:master Feb 3, 2020
@mcollina
Copy link
Copy Markdown
Member

mcollina commented Feb 3, 2020

Thanks!

This was referenced Feb 20, 2020
Merged
Closed
@Eomm Eomm mentioned this pull request Mar 14, 2020
Merged
4 tasks
This was referenced Mar 20, 2020
Closed
Merged
@snyk-bot snyk-bot mentioned this pull request Mar 29, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request Apr 12, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request May 5, 2020
Closed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 10, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mcollina mcollina mcollina approved these changes

@Eomm Eomm Eomm approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

+1 more reviewer

@tests-checker tests-checker[bot] tests-checker[bot] requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugfix Issue or PR that should land as semver patch v2.x Issue or pr related to Fastify v2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Promise rejection without error

5 participants

@Ravit436 @mcollina @allevo @Eomm @RafaelGSS