-
-
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: set status on error with progressive fallback #11054
Conversation
🦋 Changeset detectedLatest commit: 7fa8656 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 |
Tests are failing consistently 🤔 |
return new Response(response.body, { | ||
status: result.error.status, | ||
statusText: result.error.message, | ||
headers: response.headers, | ||
}) |
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.
Maybe we should return a simplified statusText
for prod? In #10284, it was considered a vulnerability that the error message was shown to the users.
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.
Good point. I also realized statusText
does not support special characters like newlines. To keep things simple, I'll just out the error.name
here, which is just the status code written out like NOT_FOUND
. No need for prod security checks I don't think
b6442fd
to
770c771
Compare
Changes
Respect error status when using a progressive fallback. This will overwrite any
status
set from Astro frontmatter withresponse.status
. Curious if we think this makes sense?Testing
Already have e2e test for progressive fallback validation errors. It doesn't check the status, but this is difficult to assert.
Docs
N/A