Skip to content

Implement patchAsyncStorage() #158

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

Closed
vicb opened this issue Dec 10, 2024 · 6 comments
Closed

Implement patchAsyncStorage() #158

vicb opened this issue Dec 10, 2024 · 6 comments

Comments

@vicb
Copy link
Contributor

vicb commented Dec 10, 2024

Open next relies on patchAsyncStorage() to select the React bundle to load.

We should implement the same functionality for the cloudflare adapter where we can not patch mod._resolveFilename

@vicb vicb converted this from a draft issue Dec 10, 2024
@IgorMinar
Copy link
Contributor

Is this to support different version of React for pages vs app router parts of one app?

Off topic, but related: how common is it for people to use two versions of react in a Next.js app?

@vicb
Copy link
Contributor Author

vicb commented Jan 9, 2025

This should not be require on Next14+
See this comment from @conico974

@conico974
Copy link
Collaborator

The patch for selecting the react bundle is not on patchAsyncStorage but here https://github.com/opennextjs/opennextjs-aws/blob/8530f7c057235a2627575838f9b05bffdc5ffb66/packages/open-next/src/core/util.ts#L28-L29.
The patchAsyncStorage patch is to fix an issue with the fetch cache not working properly on ISR request opennextjs/opennextjs-aws#503 because of this line https://github.com/vercel/next.js/blob/704a24a7ed326ed2faa737942d19c504104f57df/packages/next/src/server/lib/patch-fetch.ts#L767.
The better fix for this would be to fix this line directly in the next bundled chunks or this vercel/next.js#72082 (i need to test this with the new use cache system ideally)

Is this to support different version of React for pages vs app router parts of one app?

Yes, app and page router could use 2 different version of react, and for next >13.4.13 and <13.5.1 we had to patch it ourselves per request because NextServer was expected to run on different instance depending on if you used page or app router. (In standalone it was running in separate jest worker)

Off topic, but related: how common is it for people to use two versions of react in a Next.js app?

Not very common, it's for people that uses both page and app router (and using different version). Now that react 19 is out i guess you could use the same version for both

@vicb vicb changed the title Select React bundle (ON patchAsyncStorage()) Implement patchAsyncStorage() Feb 9, 2025
@vicb
Copy link
Contributor Author

vicb commented Feb 10, 2025

@dario-piotrowicz

I'm not sure to correctly understand your comment on the aws repo

I wonder why my change was working in our adapter (unless our adapter was throwing but swallowing the errors?! 🤔)

Do you mean that your change is actually fixing something?
(my understanding is that _resolveFilename is never called by the cloudflare adapter because require are no resolved at runtime - the code would throw if the function was called because it is not implement in unenv).

(however it is a bit too bad that aws does something that then we need to un-do, I wonder if that could be solved somehow at some point 😕)

Patching at build time would work for both the adapters so I think it is something that can be done in aws.

@dario-piotrowicz
Copy link
Contributor

Do you mean that your change is actually fixing something? (my understanding is that _resolveFilename is never called by the cloudflare adapter because require are no resolved at runtime - the code would throw if the function was called because it is not implement in unenv).

Nono I wasn't saying that my change was fixing something, I was just wondering why we were not getting any error, if the code when run in our adapter never calls _resolveFilename then that would explain why we were not getting any error 🙂

@vicb
Copy link
Contributor Author

vicb commented Mar 19, 2025

Will be fixed by opennextjs/opennextjs-aws#783

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

No branches or pull requests

4 participants