Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

# for free to join this conversation on GitHub. Already have an account? # 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