-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
feat: cloudflare pages adapter, alternate implementation #795
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
2175ac0
to
9cde263
Compare
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Maybe we just go with this PR - especially if it doesn't require setting the base path in waku config (I also just want to use Waku with pages asap haha) Looks like significant effort has gone into getting a dev environment set up, look forward to trying it! |
9cde263
to
5d39371
Compare
5d39371
to
36c40f8
Compare
It seems like it would be helpful to add some docs about setting up wrangler dev server middleware and deploying to cloudflare. Maybe at the docs/builder/cloudflare.mdx location? Or maybe we should also add a new example with a cloudflare pages project that has an environment variable binding? I used this to verify it works. In wrangler.toml:
In a page component:
Outputs |
I can't get a build to work while my waku dev server middleware is in place without marking wrangler and miniflare as external in a custom vite.config.ts:
Everything works now for dev, build and deploy. I thought the dynamic import would get it to not be included when building, but it doesn't seem to be the case with the current build system. |
We might want to document that dependencies with wasm files probably need to be added to |
Are you familiar with Vite and its bundling quirks? I've been trying to migrate an Astro React app to Waku and by far the biggest challenge I'm facing is lots of packages that work fine in Astro get tripped up with Waku's Vite setup. (I realise this is getting way off-topic; just wanted to provide tangible examples of challenges with Vite. I might start a discussion on this.) Here's another example: I want to use the lib Would be great to hear if you've any insights regarding Waku's Vite internals |
I reworked my dev server plugin to be in an external file and adjusted the dynamic importing in the waku.config.ts. That got it working without needing to externalize those packages in vite. I moved it to a gist rather than being pasted in the description of this issue. https://gist.github.com/rmarscher/9bb6ed54dc9535f4b81bed147204c7e9 I think the transition from CommonJS to ESM -- and different module resolution rules between versions of javascript, node and typescript -- has been one of the most painful things for the javascript ecosystem. It sounds like we need a separate issue or discussion to work out the problem building that package and others like it. |
Re:
I found there's an open bug about fixing that warning - cloudflare/workers-sdk#6011 |
// if (opts.serve === 'cloudflare') { | ||
// viteConfig.build.rollupOptions.external.push( | ||
// 'hono/cloudflare-workers', | ||
// '__STATIC_CONTENT_MANIFEST', | ||
// ); | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simply remove it instead of commenting out. (or maybe it's WIP)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dai-shi Thank you. I forgot to remove the commented code once I confirmed it was no longer needed. I added another commit to this PR to remove it.
@@ -59,18 +59,18 @@ export function rscServePlugin(opts: { | |||
opts.distPublic, | |||
), | |||
}; | |||
if (opts.serve === 'cloudflare' || opts.serve === 'partykit') { | |||
if (opts.serve === 'partykit') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@threepointone you might be interested in this PR, as you followed the previous cloudflare impl?
I'll have a look this week! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me.
We would like to have reviews from @jkhaui and @threepointone .
At a glance, it looks good - I can see similar patterns used in Hono's Vite CFP plugin which gives confidence this is the right way! I'll give it a proper review later today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -59,18 +59,18 @@ export function rscServePlugin(opts: { | |||
opts.distPublic, | |||
), | |||
}; | |||
if (opts.serve === 'cloudflare' || opts.serve === 'partykit') { | |||
if (opts.serve === 'partykit') { | |||
viteConfig.build ||= {}; | |||
viteConfig.build.rollupOptions ||= {}; | |||
viteConfig.build.rollupOptions.external ||= []; | |||
if (Array.isArray(viteConfig.build.rollupOptions.external)) { | |||
viteConfig.build.rollupOptions.external.push('hono'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't hono
still need to be externalised here for the CFP build? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us know if we need it and we can have a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine without (didn't experience any build or dev issues without it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks fine! Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, guys. Let's merge.
Also, if there's room for improvement for partykit
, feel free to send a PR.
@@ -59,18 +59,18 @@ export function rscServePlugin(opts: { | |||
opts.distPublic, | |||
), | |||
}; | |||
if (opts.serve === 'cloudflare' || opts.serve === 'partykit') { | |||
if (opts.serve === 'partykit') { | |||
viteConfig.build ||= {}; | |||
viteConfig.build.rollupOptions ||= {}; | |||
viteConfig.build.rollupOptions.external ||= []; | |||
if (Array.isArray(viteConfig.build.rollupOptions.external)) { | |||
viteConfig.build.rollupOptions.external.push('hono'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us know if we need it and we can have a follow-up PR.
Note to anyone attempting to deploy to Cloudflare Pages, use the command npx wrangler pages deploy Although it is not mentioned in the PR and document, it does work. |
Can anyone update README? |
Sure. Shall I open a PR? |
Yes. |
Just playing with Waku today, was initially quite surprised it wasn't defaulting to Cloudflare Pages. Then, I found this PR, upgraded Waku to So thanks for shipping this, Pages gives a much better experience at the moment 🙏 |
@jkhaui I forgot about this. Waiting for your PR. |
78a5503 is this correct and enough? |
Can anyone help #869? |
Here is an alternate attempt at Cloudflare Pages support. Related to #793.
worker.js
(a directory, not a file). It creates a dist/worker.js/index.js file to load thedist/worker.js/serve.js
file.Reference:
A waku plugin can be used to make wrangler dev server bindings available.
This could be bundled into waku or an external plugin... but it would want the developer to supply the wrangler version as a peer dependency. Cloudflare is constantly publishing wrangler and miniflare versions with new features. I published an example in a gist: https://gist.github.com/rmarscher/9bb6ed54dc9535f4b81bed147204c7e9
I published examples/10_fs-router to https://c03fa4b1.waku-test.pages.dev/
Thanks for waku! I'm looking forward to trying it on my cloudflare pages projects!