-
Notifications
You must be signed in to change notification settings - Fork 1k
Support Next.js in autoconfig #11301
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 8aa1a3b The changes in this PR will be included in the next version bump. 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 |
e1a384a to
31e5296
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
31e5296 to
20d1242
Compare
20d1242 to
a2f24b5
Compare
b1d0f1b to
ba94707
Compare
a2f24b5 to
a087f96
Compare
a087f96 to
db411f0
Compare
|
Converting to draft since the C3 e2es don't currently run in CI and we should enable them before merging and new C3 autoconfig PR |
db411f0 to
9845bde
Compare
|
When I tried this locally, with a new project, I got "✘ [ERROR] The entry-point file at ".open-next/worker.js" was not found.". I needed to do another |
|
The issue is that it doesn't run |
|
Should it change the build command? |
69a48af to
73871b5
Compare
oh... yeah I see the issue.... that's annoying.... I don't think we could change the |
73871b5 to
0927724
Compare
|
I think these changes should be in a separate PR. Will they then override the package.json build command too? |
No, because as I said I think we likely don't want that since it's just likely going to create issues (and either way we'd need a build script (even if named different) that we can point open-next to). Here wrangler just builds the app using npx (and analogous) in the This to me sounds like an appropriate first implementation that we can improve later on how/if needed |
2a9adfd to
abd3263
Compare
abd3263 to
88b8055
Compare
|
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
88b8055 to
f6f7300
Compare
| directory: ".open-next/assets", | ||
| }, | ||
| }, | ||
| buildCommand: `${npx} @opennextjs/cloudflare 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.
Why is this different to opennextjs-cloudflare?
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.
Also, the next docs don't recommend changing this: https://opennext.js.org/cloudflare/get-started
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.
Why is this different to
opennextjs-cloudflare?
Here, having
buildCommand: `opennextjs-cloudflare build`,
doesn't work and it errors that it can't recognize the binary name opennextjs-cloudflare
I think that that is only accessible via scripts in the package.json
Also, the next docs don't recommend changing this: https://opennext.js.org/cloudflare/get-started
opennextjs-cloudflare points to the package binary, so it's an alias for @opennextjs/cloudflare so running one or the other shouldn't make much of a difference... no? 🤔
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.
Sorry, what I meant is that the open next docs seem to recommend using next build here
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.
mh.... ok sorry I think I understood the confusion now... (and my unclear code caused it)
The package.json's build script should definitely remain next build, and I am not changing that in this PR
What I was passing here is a build command, that autoconfig needs to run to build the project (the build command being a completely different concept to the build script, buildCommand !== packageJson.scripts.build)...
did I get it right? is this were the confusion lied here? did you think I was updating the build script?
| /** | ||
| * Base class for errors where something in a autoconfig frameworks' configuration goes | ||
| * something wrong. These are not reported to Sentry. | ||
| */ |
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.
Is this not just a UserError? Why do we need a new class?
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 might come in handy to know whether an error comes from the configuration step or not (with a simple instanceof check). This can also be pretty convenient for potential users of the programmatic API.
I also think this will be nicer as part of the programmatic API (e.g. a consumer of the programmatic API providing their own framework object).
If you disagree I'm happy to go with just UserError errors instead.
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.
That's fair! What about making it extend UserError instead of re-implementing 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.
ok sure 🙂 👍
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.
Updated 🙂
| const { npx } = await getPackageManager(); | ||
|
|
||
| return { | ||
| wranglerConfig: { |
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 open next docs mention a WORKER_SELF_REFERENCE service binding. Do we need that? https://opennext.js.org/cloudflare/get-started
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.
It's not in the C3 starter template... so I think not? 🤔
Co-authored-by: Somhairle MacLeòid <[email protected]>
07486f2 to
8aa1a3b
Compare
Fixes https://jira.cfdata.org/browse/DEVX-2321