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
Run onSend hooks when an encapsulated handler invokes the notFound handler #870
Conversation
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.
How much this affects benchmarks (404 with onSend hooks)?
Can we pre-compute the list instead? Adding the hooks to the 404 context as well during boot?
lib/reply.js
Outdated
reply.context = reply.context._404Context | ||
reply.context.handler(reply.request, reply) | ||
// Use a copy of the 404 context with the original context's onSend hooks | ||
const context = Object.assign({}, reply.context._404Context) |
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.
Can you use prototypical inheritance here (Object.create..)? So that we avoid a full copy, the context is big and assign is slow.
lib/reply.js
Outdated
// Use a copy of the 404 context with the original context's onSend hooks | ||
const context = Object.assign({}, reply.context._404Context) | ||
context.onSend = reply.context.onSend | ||
reply.context = context |
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.
Can you make all this block optional, only if there are onSend hooks?
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
Can you also upper case the F
here?
Line 689 in b010287
reply.code(404).send(new Error('Not found')) |
In this way the message is the same as the spec.
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. |
If a route is encapsulated, it may have more
onSend
hooks than the notFound handler associated with the route. This PR makes sure that if a route invokes the notFound handler, all of the route'sonSend
hooks will run.Fixes #868
Checklist
npm run test
andnpm run benchmark