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

feat(remix-server-runtime): strip body from request before calling loaders #3207

Merged
merged 2 commits into from
May 17, 2022

Conversation

jacob-ebey
Copy link
Member

@jacob-ebey jacob-ebey commented May 16, 2022

chore: remove duplicate method definition

Since loaders map to "GET" and "HEAD" requests, those by definition can't have a body and therefore there is no reason to pass it in to use-code. This also avoids exclusive locks on the body when multiple loaders are executed in parallel.

Changes:

  • Action recieves OG request and can choose to read the body.
  • handleDocumentRequest recieves OG request, if the action read the body it will be locked and bodyUsed will be true. Atempting to read the body again will throw
  • Loaders recieve a copy of the OG request without the body.

Closes: #3003

  • Docs
  • Tests

chore: remove duplicate method definition

Since loaders map to "GET" and "HEAD" requests, those by definition can't have a body and therefore there is no reason to pass it in to use-code. This also avoids exclusive locks on the body when multiple loaders are executed in parallel.

Changes:
- Action recieves OG request and can choose to read the body.
- handleDocumentRequest recieves OG request, if the action read the body it will be locked and `bodyUsed` will be true. Atempting to read the body again will throw
- Loaders recieve a copy of the OG request without the body.
@MichaelDeBoey MichaelDeBoey changed the title feat: strip body from request before calling loaders feat(remix-server-runtime): strip body from request before calling loaders May 16, 2022
@chaance
Copy link
Collaborator

chaance commented May 17, 2022

@jacob-ebey any chance we could get a test case in here?

@jacob-ebey
Copy link
Member Author

@chaance I would love to but none of this is broken on node and we don't have a way to easily run integration tests against other runtimes. This is a CF / Deno only issue.

@chaance
Copy link
Collaborator

chaance commented May 17, 2022

I would love to but none of this is broken on node

👍

we don't have a way to easily run integration tests against other runtimes

😭

@chaance chaance merged commit ba6de76 into dev May 17, 2022
@chaance chaance deleted the jacob/loader-request-no-body branch May 17, 2022 18:49
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v1.5.0-pre.1 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!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v1.5.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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants