-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Allow custom 404 route to handle API route missing methods #4594
Conversation
🦋 Changeset detectedLatest commit: a9ca414 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Looks like the tests are failing |
I'd suggest there be another way to handle this. Serving a 404 page might be appropriate for a user navigating with a web browser to a missing page, but it's not appropriate to respond to, for example, a DELETE request to an incorrect API URL. |
f8a83a1
to
d5119e5
Compare
d5119e5
to
2d8362a
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.
💯 this should be a 404 rather than 500, I'm a little worried about the extra response time added by using the custom 404 page though
Thoughts on returning a 404 with either an empty body or a simple error message?
@tony-sull I guess my main thought is that if we don't return the custom 404 page people will report that as a bug. I think the expectation is that all 404s go through 404.astro. I think we should prioritize correctness here. |
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 (followed up offline re: rendering 404 HTML)
Thanks @tony-sull |
…#4594) * Properly allow file uploads in the dev server * Allow custom 404 route to handle API route missing methods * Add a changeset * what was i thinking * Pass through the pathname * Move the try/catch out and into handleRequest * await the result of handleRoute
Changes
Testing
Docs
N/A, bug fix