-
Notifications
You must be signed in to change notification settings - Fork 90
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
Forward errors to Fastify instead of sending custom error pages #27
Conversation
Can you please rephrase the setNotFound problem? It might look like a bug in fastify. |
1458f84
to
f1d49a6
Compare
f1d49a6
to
e1cd038
Compare
I rewrote the information about errors and put it in a "Handling Errors" section after the "Options" section. |
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, but maybe we might want to do a bugfix release in fastify.
README.md
Outdated
[`fastify.setErrorHandler()`](https://www.fastify.io/docs/latest/Server-Methods/#seterrorhandler). | ||
|
||
If a request matches the URL `prefix` but a file cannot be found for the request, | ||
a `404 Not Found` error will be created and passed to Fastify's error handler. |
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 think this behavior might be wrong. We should really flow this through the notFoundHandler.
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.
Yes that would be better. From looking at https://github.com/fastify/fastify/blob/master/fastify.js I think one way to do that would be for fastify-static
to have access to Fastify's fourOhFour
router so that it can call fourOhFour.lookup(req, res)
. Do you agree?
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 recal (but I might be wrong, I’m from a phone) that Every plugin has its own 404 handler defined, so we should just use that.
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.
Yes you're right. I figured out how to call the 404 handler and I've updated the PR :)
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
if (err.statusCode === 403) return serve403(req, reply.res) | ||
serve500(req, reply.res) | ||
if (err.status === 404) { | ||
fastify._404Context.handler(request, reply) |
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’m not a huge fan of accessing a private propery. I think we should expose the handler through a documented method.
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.
Sure. What do you think that should look like? fastify.callNotFoundHandler(request, reply)
? Or maybe add it to the reply
object as reply.notFound()
?
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 prefer reply.notFound()
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.
Okay 👍 I'll propose this in Fastify with a PR
Fixes #17
@mcollina I hope this is what you meant as the solution to #17. Let me know if it isn't. I couldn't find any way to handle 404s withsetNotFoundHandler
so I made it clear in the docs that 404 errors will be passed to Fastify's error handler.