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

Properly handle empty body response codes in single fetch requests #12760

Merged
merged 5 commits into from
Jan 21, 2025

Conversation

brophdawg11
Copy link
Contributor

Copy link

changeset-bot bot commented Jan 16, 2025

🦋 Changeset detected

Latest commit: 754f590

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

This PR includes changesets to release 11 packages
Name Type
react-router Patch
@react-router/architect Patch
@react-router/cloudflare Patch
@react-router/dev Patch
react-router-dom Patch
@react-router/express Patch
@react-router/node Patch
@react-router/serve Patch
@react-router/fs-routes Patch
@react-router/remix-routes-option-adapter Patch
create-react-router 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

Copy link

@sebastien-comeau sebastien-comeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brophdawg11
Copy link
Contributor Author

I left that out on purpose, but incorrectly it seems. I thought document requests would always need an HTML body - and in the 304 case it would use the cached body. I missed that the browser will behave the same on 204 responses and use the current document:

indicates that a request has succeeded, but the client doesn't need to navigate away from its current page

205 behaves the same - although interestingly enough it doesn't reset any form fields.

I don't think 1xx makes sense for document requests? I'm going to leave those out and only apply them to data requests (even there I don't have a great use case for them...)

@brophdawg11 brophdawg11 merged commit 04b1a3b into dev Jan 21, 2025
8 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/empty-response branch January 21, 2025 16:08
@brophdawg11 brophdawg11 added the awaiting release This issue have been fixed and will be released soon label Jan 21, 2025
Copy link
Contributor

🤖 Hello there,

We just published version 6.28.3-pre-v6.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 7.1.4-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Comment on lines +429 to +443
// some status codes are not permitted to have bodies, so we want to just
// treat those as "no data" instead of throwing an exception.
// 304 is not included here because the browser should fill those responses
// with the cached body content.
const NO_BODY_STATUS_CODES = new Set([100, 101, 204, 205]);
if (NO_BODY_STATUS_CODES.has(res.status)) {
if (!init.method || init.method === "GET") {
// SingleFetchResults can just have no routeId keys which will result
// in no data for all routes
return { status: res.status, data: {} };
} else {
// SingleFetchResult is for a singular route and can specify no data
return { status: res.status, data: { data: undefined } };
}
}
Copy link

@clovis1122 clovis1122 Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: NVM, I misunderstood the spec. This is great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue have been fixed and will be released soon CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants