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

Replace request.clone() with new Request(...) #167

Merged
merged 2 commits into from
May 17, 2022

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented May 5, 2022

I received this warning while running in Cloudflare Workers:

Your worker called response.clone(), but did not read the body of both clones. 
This is wasteful, as it forces the system to buffer the entire response body in memory, rather than streaming it through. 
This may cause your worker to be unexpectedly terminated for going over the memory limit. 
If you only meant to copy the response headers and metadata (e.g. in order to be able to modify them), use `new Response(response.body, response)` instead

This is the only .clone() I could find in my worker, and updating this removed the warning. My app still appears to be working correctly

I received this warning while running in Cloudflare Workers:

```
Your worker called response.clone(), but did not read the body of both clones. This is wasteful, as it forces the system to buffer the entire response body in memory, rather than streaming it through. This may cause your worker to be unexpectedly terminated for going over the memory limit. If you only meant to copy the response headers and metadata (e.g. in order to be able to modify them), use `new Response(response.body, response)` instead
```

This is the only `.clone()` I could find in my worker, and updating this removed the warning. My app still appears to be working correctly
@sergiodxa
Copy link
Owner

This is needed because Remix Auth reads the body, and if you want to read it yourself later it will fail without request.clone()

@jfsiii
Copy link
Contributor Author

jfsiii commented May 6, 2022

Thanks for the explanation. This makes sense a day later. I think I interpreted this an alternative/preferred way to make a copy, but it could have just been that my app never read the .clone() body.

Closing the PR

@jfsiii jfsiii closed this May 6, 2022
@jfsiii
Copy link
Contributor Author

jfsiii commented May 6, 2022

Although, looking at https://developers.cloudflare.com/workers/examples/alter-headers

async function handleRequest(request) {
  const response = await fetch(request);

  // Clone the response so that it's no longer immutable
  const newResponse = new Response(response.body, response);

  // Add a custom header with a value
  newResponse.headers.append('x-workers-hello', 'Hello from Cloudflare Workers');

  // Delete headers
  newResponse.headers.delete('x-header-to-delete');
  newResponse.headers.delete('x-header2-to-delete');

  // Adjust the value for an existing header
  newResponse.headers.set('x-header-to-change', 'NewValue');

  return newResponse;
}

doesn't it look like these are interchangeable?

  // Clone the response so that it's no longer immutable
-  const newResponse = new Response(response.body, response);
+  const newResponse = response.clone()

Aren't these both ways duplicating a request? If so, it seems like Cloudflare has a clear preference for one.

Can you say more about the distinction you see between the two and why .clone is the preference in remix-auth?

@jfsiii jfsiii reopened this May 6, 2022
Copy link
Owner

@sergiodxa sergiodxa left a comment

Choose a reason for hiding this comment

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

I just tried and doing new Request(...) work and let you read the body of the original and the cloned request.

I can do the change so CF doesn't warn again, but I need you to change this line, it should create a new Request not a new Response.

@sergiodxa sergiodxa self-assigned this May 17, 2022
Co-authored-by: Sergio Xalambrí <hello@sergiodxa.com>
@sergiodxa sergiodxa added the enhancement New feature or request label May 17, 2022
@sergiodxa sergiodxa merged commit 50a58e0 into sergiodxa:main May 17, 2022
@john-griffin
Copy link

This change does break other environments. I'm trying to unit test an action that runs this code and new Request seems to mess up the body whereas request.clone() works fine.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants