Skip to content
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

Merged
merged 7 commits into from
Sep 14, 2022

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Sep 1, 2022

Changes

Testing

  • Tests added for dev and prod.

Docs

N/A, bug fix

@changeset-bot
Copy link

changeset-bot bot commented Sep 1, 2022

🦋 Changeset detected

Latest commit: a9ca414

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
astro Patch
@e2e/astro-component Patch
@e2e/error-cyclic Patch
@e2e/error-react-spectrum Patch
@e2e/error-sass Patch
@e2e/errors Patch
@e2e/hydration-race Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/solid-recurse Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution Patch
@e2e/third-party-astro Patch

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Sep 1, 2022
@bluwy
Copy link
Member

bluwy commented Sep 2, 2022

Looks like the tests are failing

@jazoom
Copy link

jazoom commented Sep 13, 2022

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.

Copy link
Contributor

@tony-sull tony-sull left a 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?

@matthewp
Copy link
Contributor Author

@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.

Copy link
Contributor

@tony-sull tony-sull left a 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)

@matthewp
Copy link
Contributor Author

Thanks @tony-sull

@matthewp matthewp merged commit 005d5ba into main Sep 14, 2022
@matthewp matthewp deleted the 404-wrong-method branch September 14, 2022 19:47
@astrobot-houston astrobot-houston mentioned this pull request Sep 14, 2022
mrienstra pushed a commit to mrienstra/astro that referenced this pull request Sep 14, 2022
…#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

server error on unhandled routes with no way to actually handle them
4 participants