-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
Fix loader request on document POST requests #9721
Fix loader request on document POST requests #9721
Conversation
🦋 Changeset detectedLatest commit: 8db43ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
2b39ae8
to
9315bf7
Compare
expect(rootLoaderRequest.method).toBe("GET"); | ||
expect(rootLoaderRequest.method).toBe("POST"); | ||
expect(rootLoaderRequest.url).toBe("http://localhost/child"); | ||
expect(childLoaderRequest.method).toBe("GET"); | ||
expect(rootLoaderRequest.headers.get("test")).toBe("value"); | ||
expect(await rootLoaderRequest.text()).toBe(""); | ||
expect(childLoaderRequest.method).toBe("POST"); | ||
expect(childLoaderRequest.url).toBe("http://localhost/child"); | ||
expect(childLoaderRequest.headers.get("test")).toBe("value"); | ||
// Can't re-read body here since it's the same request as the root |
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.
Because query
is a single document-level POST
request the loaders reflect POST
as well, but we don't proxy along the body
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.
Looks and sounds good but I don't think I'm familiar enough with what should be correct behavior here to approve 😅
method: request.method, | ||
redirect: request.redirect, | ||
signal: request.signal, | ||
}); |
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.
Same as Remix was doing in 1.7.6: https://github.com/remix-run/remix/blob/remix%401.7.6/packages/remix-server-runtime/server.ts#L418
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.
For the moment we are not persisting method:'POST'
as Remix did since we think that should be changed in Remix
🤖 Hello there, We just published version Thanks! |
🤖 Hello there, We just published version Thanks! |
Follow up to #9660 to ensure we proxy the
request.headers
from the action request. For document requests during SSR we also persist the method since it's a singlePOST
request
, unlike during CSR when the revalidations are separate GET requests. This matches Remix behavior in 1.7.6 but going to confirm.Remix PR: remix-run/remix#4829