next-on-pages icon indicating copy to clipboard operation
next-on-pages copied to clipboard

Remove the `cache` property from calls to `new Request(...)` in React

Open james-elicx opened this issue 9 months ago โ€ข 7 comments

In the React server code, there is a ternary where they create a new Request object and forward all properties on RequestInit to it. This won't work in workerd due to the cache property being present. Therefore, we need to overwrite their ternary with a way for us to strip the cache property from the RequestInit object.

https://github.com/vercel/next.js/blob/9ec37c12/packages/next/src/compiled/react/cjs/react.react-server.production.js#L87

Additionally, once this patch has been added, it looks like the error can then happen in our patched fetch. Therefore, also adding similar logic there to strip out the cache property from the RequestInit object.

fixes #719

james-elicx avatar May 04 '24 21:05 james-elicx

๐Ÿฆ‹ Changeset detected

Latest commit: 1f5539f325fdfe86a3d75c03d7e7171627b1a174

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@cloudflare/next-on-pages Patch
eslint-plugin-next-on-pages 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

changeset-bot[bot] avatar May 04 '24 21:05 changeset-bot[bot]

๐Ÿงช Prereleases are available for testing ๐Ÿงช

@cloudflare/next-on-pages

You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/9067847845/npm-package-next-on-pages-771

@cloudflare/eslint-plugin-next-on-pages

You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/9067847845/npm-package-eslint-plugin-next-on-pages-771

github-actions[bot] avatar May 04 '24 21:05 github-actions[bot]

Thanks a lot for the PR @james-elicx ๐Ÿ˜„

But it seems like the e2es are failing, and the failures do seem legitimate:

  • https://github.com/cloudflare/next-on-pages/actions/runs/8953245001/job/24609146706#step:4:768
  • https://github.com/cloudflare/next-on-pages/actions/runs/8953245001/job/24609146706#step:4:787

Could you please have a look? ๐Ÿ™

dario-piotrowicz avatar May 05 '24 22:05 dario-piotrowicz

argh, i was hoping a rerun might fix them. welp. that's irritating

james-elicx avatar May 05 '24 22:05 james-elicx

argh, i was hoping a rerun might fix them. welp. that's irritating

yeah no, I already did try rerunning them ๐Ÿฅฒ

Also usually they kind of either all run or all (or most) fail, when just a few tests fail like now I think that does indicate a real regression/issue ๐Ÿ˜…

dario-piotrowicz avatar May 05 '24 23:05 dario-piotrowicz

What's the status of this PR? I'm having this issue with Cloudflare pages and next-firebase-auth-edge library.

ehabsakran avatar May 26 '24 15:05 ehabsakran

What's the status of this PR? I'm having this issue with Cloudflare pages and next-firebase-auth-edge library.

i think it's probably fine to merge personally.

the area of caution is we haven't done any tests about if this would cause a regression for how the fetch cache in nextjs works. i feel like it would probably be okay though.

james-elicx avatar May 27 '24 11:05 james-elicx