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

introduce new initOpenNextCloudflareForDev utility and make getCloudflareContext synchronous #265

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz dario-piotrowicz commented Jan 17, 2025

This PR introduces a new initOpenNextCloudflareForDev utility to add to the Next.js config file that makes the getCloudflareContext work in middlewares (or more generally, the edge runtime) during local development (via next dev)

It getCloudflareContext has also been converted to be synchronous

For more details see the changeset file: .changeset/chilly-dryers-begin.md


C3 PR: cloudflare/workers-sdk#7903

Docs relative PR: opennextjs/docs#57


fixes #137
fixes #226

Copy link

changeset-bot bot commented Jan 17, 2025

🦋 Changeset detected

Latest commit: 24fa5a9

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

This PR includes changesets to release 1 package
Name Type
@opennextjs/cloudflare Minor

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

@dario-piotrowicz dario-piotrowicz force-pushed the dario/226/cloudflare-context-middleware branch from 2447a8a to f6510ac Compare January 17, 2025 19:05
Copy link

pkg-pr-new bot commented Jan 17, 2025

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/cloudflare@265

commit: 26b9119

@dario-piotrowicz dario-piotrowicz force-pushed the dario/226/cloudflare-context-middleware branch 4 times, most recently from 6648525 to 09b33de Compare January 17, 2025 22:39
examples/middleware/e2e/base.spec.ts Outdated Show resolved Hide resolved
packages/cloudflare/src/api/cloudflare-context.ts Outdated Show resolved Hide resolved
examples/middleware/next.config.mjs Outdated Show resolved Hide resolved
packages/cloudflare/src/api/cloudflare-context.ts Outdated Show resolved Hide resolved
packages/cloudflare/src/api/cloudflare-context.ts Outdated Show resolved Hide resolved
packages/cloudflare/src/api/cloudflare-context.ts Outdated Show resolved Hide resolved
packages/cloudflare/src/api/cloudflare-context.ts Outdated Show resolved Hide resolved
@vicb
Copy link
Contributor

vicb commented Jan 20, 2025

Thought: Maybe we can getPlatformProxy(...) in initOpenNextCloudflare() so that getCloudflareContext could be made sync?

This comment says "the function is an async one but it doesn't need to be awaited", it might be nice to add the rationale if we add the implementation to our repo.

@dario-piotrowicz dario-piotrowicz force-pushed the dario/226/cloudflare-context-middleware branch from 09b33de to 59419ca Compare January 20, 2025 10:58
@dario-piotrowicz
Copy link
Contributor Author

Thought: Maybe we can getPlatformProxy(...) in initOpenNextCloudflare() so that getCloudflareContext could be made sync?

Yes, we could, this would mean that everyone would always have to call initOpenNextCloudflare(), I mentioned in my comment that we don't need to go that route as not forcing users to do that might provide a better DX

but again if you strongly prefer us to go that route I'm ok with it

This comment says "the function is an async one but it doesn't need to be awaited", it might be nice to add the rationale if we add the implementation to our repo.

Yes, given the implementation of next dev, it takes some time from when Next.js loads the config and when it serves the app, we did see that that amount of time is enough for the proxy to be ready in time. It would require very little changes here, just removing the on-the-fly proxy generation from getCloudflareContext and changing the function signature to be sync

@dario-piotrowicz dario-piotrowicz requested a review from vicb January 20, 2025 11:48
@dario-piotrowicz
Copy link
Contributor Author

dario-piotrowicz commented Jan 20, 2025

@vicb I've addressed all the feedback, the only thing left is to decide whether we want to force this sort of thing via a initOpenNextCloudflare() and potentially convert getCloudflareContext to be sync or not

up to you, just let me know if you want to me make the change 👍
(it doesn't really take much effort to do code-wise, it's just a matter of DX)

@vicb
Copy link
Contributor

vicb commented Jan 20, 2025

@vicb I've addressed all the feedback, the only thing left is to decide whether we want to force this sort of thing via a initOpenNextCloudflare() and potentially convert getCloudflareContext to be sync or not

up to you, just let me know if you want to me make the change 👍 (it doesn't really take much effort to do code-wise, it's just a matter of DX)

Give me some time to think about it, I'm not settled.

Some thoughts I have for now about initOpenNextCloudflare()

Cons:

initOpenNextCloudflare() always need to be called but we can have an error message when it's not called

Pros

  • I think it's easier to document (i.e. "always call initOpenNextCloudflare()" vs " call enableEdgeDevGetCloudflareContext if you call getConflareContext from a middleware"). I think Vercel is working on the ability to run middlewares on the node runtime and we will add support for routes on the edge runtime at some point. It will make it harder to explain
  • I think it's easier for users to understand (They don't have to understand what "EdgeDev" is)
  • We could setup the cloudflare context in there so that getCloudflareContext() becomes sync. A lot of user are complaining about that on Discord and GH - See how it impacts us in the KVCache
  • Calling the init on a per need basis (vs always calling initOpenNextCloudflare()) could introduce unrelated test failures when you start adding the call - i.e. how you had to bump the timeout in this PR

@dario-piotrowicz
Copy link
Contributor Author

Give me some time to think about it, I'm not settled.

Some thoughts I have for now about initOpenNextCloudflare()

...

Well there is only one con and many pros, I dislike that we need to then ask users to always call the function, but given the benefits it does seem like it might be worth it

By the way, either way I am not too convinced about the name since this is a dev (next dev) only thing and in my opinion extremely unlikely to be useful outside of dev (because we have other places where we could accept options/tweak things, like in our templates, in the open-next config, etc...) for this reason I would really prefer it if the function had dev somewhere in its name (e.g. initOpenNextCloudflareForDev())

@dario-piotrowicz dario-piotrowicz marked this pull request as draft January 23, 2025 12:45
@dario-piotrowicz dario-piotrowicz force-pushed the dario/226/cloudflare-context-middleware branch from 1c298b1 to b234e3e Compare January 23, 2025 18:28
@dario-piotrowicz dario-piotrowicz changed the title enable getCloudflareContext to work in middlewares via a new enableEdgeDevGetCloudflareContext utility introduce new initOpenNextCloudflareForDev utility and make getCloudflareContext synchronous Jan 23, 2025
@dario-piotrowicz dario-piotrowicz force-pushed the dario/226/cloudflare-context-middleware branch from b234e3e to d7f6572 Compare January 23, 2025 18:39
@dario-piotrowicz dario-piotrowicz marked this pull request as ready for review January 23, 2025 18:54
@dario-piotrowicz dario-piotrowicz force-pushed the dario/226/cloudflare-context-middleware branch from ea44365 to 6006cf2 Compare January 24, 2025 00:47
@vicb
Copy link
Contributor

vicb commented Jan 24, 2025

I've pushed the kvCache update

@vicb
Copy link
Contributor

vicb commented Jan 24, 2025

Could you please update the README and opennext.js.org to tell users to add the init?

@dario-piotrowicz dario-piotrowicz requested a review from vicb January 24, 2025 18:12
Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Could you please update https://github.com/opennextjs/opennextjs-cloudflare/blob/main/packages/cloudflare/README.md

Either to ask the user to add the call to the init function
Or if you can update opennext.js.org, remove the content and link to the doc site (I was mostly using the content for the experimental branch when the experimental features were not documented anywhere else)

Great stuff, thanks 🚀

.changeset/chilly-dryers-begin.md Outdated Show resolved Hide resolved
packages/cloudflare/src/api/cloudflare-context.ts Outdated Show resolved Hide resolved
.changeset/chilly-dryers-begin.md Outdated Show resolved Hide resolved
Co-authored-by: Victor Berchet <victor@suumit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants