-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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: call preHandler on reply.callNotFound #2256
Conversation
26035fc
to
51cd64f
Compare
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.
Hello! Thank you for the pr.
I think we should reuse the preHandlerCallback
present in lib/handleRequest.js
Lines 107 to 133 in dce32e7
function preHandlerCallback (err, request, reply) { | |
if (reply.sent || | |
reply.raw.writableEnded === true || | |
(reply.raw.writable === false && reply.raw.finished === true)) return | |
if (err != null) { | |
reply.send(err) | |
return | |
} | |
var result | |
try { | |
result = reply.context.handler(request, reply) | |
} catch (err) { | |
reply.send(err) | |
return | |
} | |
if (result !== undefined) { | |
if (result !== null && typeof result.then === 'function') { | |
wrapThenable(result, reply) | |
} else { | |
reply.send(result) | |
} | |
} | |
} |
As you can see we are also doing more work, namely the synchronous error checking.
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
I expect the automatic backport to fail since the |
* fix: run preHandler in notFound * doc: update docs for setNotFoundHandler * fix: reuse preHandlerCallback
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. |
Fixes #2229
Checklist
npm run test
andnpm run benchmark