Make site work with the Cloudflare OpenNext adapter#7383
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dario-piotrowicz
force-pushed
the
dario/opennextjs-cloudflare
branch
from
b5e1217 to
0463451
Compare
|
@dario-piotrowicz do we have updates here? 👀 |
|
Hey @ovflowd 👋 Sorry for keeping the PR lingering, there are a few smaller issues we addressed in our adapter (that I need to reflect here), and also ISR should be coming soon (@vicb can provide more context) Besides that I just need to rebase the PR, the only significant issue remaining should be filesystem access, but I wanted to clarify that with you, I'll drop you a message today to clarify things (PS: I hope the PR is not bothering you 🙇, if you want I can close it and reopen it when we're ready?) |
This is awesome news. Excited to hear from @vicb
I believe we sorted that out on Slack 🖖
Not at all <3 |
dario-piotrowicz
force-pushed
the
dario/opennextjs-cloudflare
branch
2 times, most recently
from
542f172 to
2723110
Compare
dario-piotrowicz
force-pushed
the
dario/opennextjs-cloudflare
branch
from
2723110 to
3481f56
Compare
dario-piotrowicz
force-pushed
the
dario/opennextjs-cloudflare
branch
from
3481f56 to
f746d5c
Compare
dario-piotrowicz
force-pushed
the
dario/opennextjs-cloudflare
branch
from
f746d5c to
4ff9d79
Compare
dario-piotrowicz
force-pushed
the
dario/opennextjs-cloudflare
branch
from
4ff9d79 to
fac63da
Compare
|
Once #7383 (comment) #7383 (comment) resolve, this can probably land 🎉 |
|
However, I would love if there was a test in the CI to verify this builds for any given push/PR. |
mh... that's an interesting idea.... we've considered PR previews + e2es but not something that purely tests the build aspect of this 🤔 I wouldn't be against it, I am not sure how much extra confidence that would give us, but it'd definitely not hurt either (besides the potential extra noise etc...) I'm totally happy to look into that 🙂 |
|
It can totally be a follow up, I just don't want us to have the unfortunate scenario where PR deploys successfully on Vercel, but not on OpenNext. |
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
LGTM after my final comments get addressed
Thanks Claudio 😃 There's still #7383 (comment) outstanding, please give it a quick look (it should be rather minor in any case) |
|
Did you link the right discussion? That one’s resolved with the conclusion that no changes are needed. |
Nope, sorry I pointed to the wrong one, thanks for spotting it, I've updated the link 🙂👍 |
|
I've resolved #7383 (comment). With that in mind, I'd love to merge this tomorrow (or sooner, if you'd like) if there are no objections1. WDYT? Footnotes
|
update the site application so that it can be build using the Cloudflare OpenNext adapter (`@opennextjs/cloudflare`) and thus deployed on Cloudflare Workers
Thank you very much @avivkeller 🫶 With that resolved I think this should (finally! sorry about how long this took! 😅) be ready to be merged! 😄 🥳 I'm also happy to merge this as long as there are no objections 😃 |
|
It's your PR, so feel free to merge whenever you'd like. Honestly, Any changes could be done in a follow up. |
|
Finally made it! thanks a lot everyone for the reviews and the support on this one! 🫶 |
|
This PR worries me; It got 0 approvals from web-infra when it is pretty much an infrastructure PR. Ideally this should have gotten an approval from at least me, @bmuenzenmeyer or @flakey5 or @MattIPv4 or @canerakdas. But it got none :/ |
|
Anyhow, I believe my concerns can be addressed in a follow-up PR; I'd just really appreciate if they get done; I wouldn't mind doing'em myself to be honest. |
|
That's on me. I encouraged the merging of this PR - and should've checked that web-infra got a chance to review. Which of your concerns weren't addressed? I think the postinstall concern is resolved (Postinstall scripts are no longer in use). |
That's OK; My concern was a base template file existing; Otherwise if I never run any of the run commands, or just do I still wonder if we accidentally have too much complexity on the repository. I truly think we gotta audit the repository's complexity and attempt to trim it down. (Monorepo aside, cause that's fine) |
That's definitely on me as well (more than you I'd argue) 🥹 , sorry @ovflowd 🙇 , given your last approval (note that I addressed the comments that approval refers to) which is from a web-infra folk and the rest of the approvals I didn't really see a concern about the merge. Yes I agree that this probably should have gotten some more web-infra visibility 🙇 |
Indeed, I didn't realise I approved; That's on me, apologies for calling out there was no approval when there was indeed one. It's all good, and it's minor :) Anyhow, I'm super proud of the work here ✨ |
Thanks! 🫶 (But again, sorry, I should have checked with you before merging 🙇)
Thanks, and thanks for all the help with landing this 🫶 |
@ovflowd as I mentioned in the conversation there I did try what you suggested but that didn't work, and on the top of my head I can't think of a way to make it work without a postinstall script, I'm happy to help here but I am not sure how, would you mind giving that a quick try on your end? 🙏 |
8 participants
This PR applies changes to make it so that the site can be deployed to Cloudflare workers using the open-next Cloudflare adapter
The app does seem to work as intended for the most part:
Deployment URL: https://nodejs-website.web-experiments.workers.dev
Check List
npm run formatto ensure the code follows the style guide.npm run testto check if all tests are passing.npx turbo buildto check if the website builds without errors.