-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Refactor 404 and 500 approach #7754
Conversation
🦋 Changeset detectedLatest commit: da6d43a The changes in this PR will be included in the next version bump. 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 |
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 like this approach a lot. Great refactor!
ddbdf29
to
896a5e8
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.
A few updates! Moved some tests around, added a couple small fixes
a753333
to
70bb342
Compare
* If is a known error code, try sending the according page (e.g. 404.astro / 500.astro). | ||
* This also handles pre-rendered /404 or /500 routes |
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.
* If is a known error code, try sending the according page (e.g. 404.astro / 500.astro). | |
* This also handles pre-rendered /404 or /500 routes | |
* If it is a known error code, try sending the according page (e.g. 404.astro / 500.astro). | |
* This also handles pre-rendered /404 or /500 routes |
const { status, statusText, headers } = oldResponse; | ||
|
||
return new Response(newResponse.body, { | ||
status: status === 200 ? newResponse.status : status, |
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 is weird; why do we return newRespnose.status
if we check the original staus
? Can you add a comment?
@natemoo-re I left some feedback, could you please address it? |
Changes
app
to handle404
and500
routes more consistently404
and500
pages by making a new request to the origin serverTesting
Tests updated!
Docs
N/A