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 ready order execution #2333
Conversation
lib/hooks.js
Outdated
exit(err) | ||
done(err) |
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 don't understand. This looks like two separate callbacks being invoked. How is this deterministic?
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.
Here we are appending a function to avvio's ready function.
This is needed to catch the timeout exception and bubble the error up to the user.
The done, is the avvio callback and it must be called to continue the loading of the application
The exit function, instead it is the a local function that will append the onready hooks of all the children in the fastify instance tree and, when the is an error it will stop appending the hooks and returns to the user the error
So this function will be executed every time as the last function for every fastify context.
This has been done to provide to the fastify onready hook a signature without the error handling in charge of the user
Maybe a comment could be helpful?
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.
Maybe a comment could be helpful?
I think so.
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 agree with @jsumners :)
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.
Done
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.
lgtm
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.
lgtm
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.
LGTM
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. |
This test was failing in v3: the order was reverted
Checklist
npm run test
andnpm run benchmark