sergiodxa / remix-auth

Simple Authentication for Remix
MIT License
1.94k stars 112 forks source link

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

Closed jfsiii closed 2 years ago

jfsiii commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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?

john-griffin commented 1 year ago

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.