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

add "async mode" to getCloudflareContext #372

Merged
merged 20 commits into from
Feb 14, 2025

Conversation

dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz dario-piotrowicz commented Feb 12, 2025

add a new option to getCloudflareContext that makes it run in "async mode", the difference being that the returned value is a promise of the Cloudflare context instead of the context itself

The main of this is that it allows the function to also run during SSG (since the missing context can be created on demand).


Closes #371

Docs PR: opennextjs/docs#75

add a new option to `getCloudflareContext` that makes it run in "async mode", the difference being that the returned value is a
promise of the Cloudflare context instead of the context itself

The main of this is that it allows the function to also run during SSG (since the missing context can be created on demand).
Copy link

changeset-bot bot commented Feb 12, 2025

🦋 Changeset detected

Latest commit: 621718e

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 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

Copy link

pkg-pr-new bot commented Feb 12, 2025

Open in Stackblitz

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

commit: 621718e

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.

Added a few inline comments.

I'll take more time to review more closely tomorrow

examples/ssg-app/app/page.tsx Outdated Show resolved Hide resolved
@dario-piotrowicz
Copy link
Contributor Author

@vicb sorry I had to update the logic in getCloudflareContext as I realized that in some cases users could get the wrong behavior (a wrong error being thrown), sorry for the inconvenience please have another look 🙏


PS: ideally all the different error cases of this (and their error messages) should be tested, for that we would need an extra app that doesn't set initOpenNextCloudflareForDev, also the build SSG aspects can't be properly tested anyways: #331. So it doesn't feel worth it to me (at least not right now, maybe after 331), especially since if things would go wrong at worst users could simply get the wrong error message.
I'm happy to add some tests here or in a followup if you disagree and thing that they'd be worth it.

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.

LGTM

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 vicb force-pushed the dario/371/getCloudflareContext-async branch from 4e509bc to 30683e7 Compare February 14, 2025 06:04
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.

Maybe:

const InitOpenNextCloudflareForDevErrorMsg =`...`;
...
throw new Error(InitOpenNextCloudflareForDevErrorMsg);

@vicb vicb merged commit 522076b into main Feb 14, 2025
7 checks passed
@vicb vicb deleted the dario/371/getCloudflareContext-async branch February 14, 2025 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getCloudflareContext async
2 participants