Wrangler: update autoconfig logic to handle Next.js projects by using the new @opennextjs/cloudflare migrate command#12190
Conversation
… the new `@opennextjs/cloudflare migrate` command
🦋 Changeset detectedLatest commit: fe581f5 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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: |
| dryRunConfigurationResults?.wranglerConfig === null | ||
| ? null | ||
| : { | ||
| ...wranglerConfig, | ||
| ...dryRunConfigurationResults?.wranglerConfig, | ||
| }, |
There was a problem hiding this comment.
What about the scenario where dryRunConfigurationResults === undefined? Should it still show the wranglerConfig?
I guess what we are saying (at least how it worked before) is that if the dryRunConfigurationResults is undefined then we are just using the default one (i.e. wranglerConfig)?
There was a problem hiding this comment.
yes, dryRunConfigurationResults can be undefined only when a framework is not detected, in that case we want to generate the default wrangler config.
(PS: at this point anyways dryRunConfigurationResults cannot be undefined since we'd have detected a framework by now (even if just the Static one), I think the types here could be improved...)
| // If wranglerConfigToWrite is null the framework's tool is generating the wrangler config so we don't know | ||
| // if there is server side code or not, but likely there should be so we here assume that there is |
There was a problem hiding this comment.
This comment is quite verbose. Can it be simplified?
| }, | ||
| ], | ||
| }, | ||
| // `@opennextjs/cloudflare migrate` creates the wrangler config file |
There was a problem hiding this comment.
I don't really understand why we're special casing Next here? Other framework adapt commands generate a wrangler.json, and autoconfig still works fine (adding a few default fields but otherwise being entirely overwritten by the framework fields). Was next broken without this change?
Does Next add $schema and observability?
There was a problem hiding this comment.
I don't really understand why we're special casing Next here? Other framework adapt commands generate a wrangler.json, and autoconfig still works fine (adding a few default fields but otherwise being entirely overwritten by the framework fields)
If frameworks generate wrangler.jsonc files then I think we should revisit that, as we currently override it:
workers-sdk/packages/wrangler/src/autoconfig/run.ts
Lines 177 to 184 in 09a8829
If a framework generates its own wrangler.jsonc config file then I think we should either honor it (as I am doing in this PR) or editing it, not completely overriding it.
Was next broken without this change?
Yes, the openNext migrate command adds various fields in the config (https://github.com/opennextjs/opennextjs-cloudflare/blob/6727373c31600b627e3c633b9bc4d24013927904/packages/cloudflare/templates/wrangler.jsonc#L11-L34) and with the old logic we were completely deleting them
There was a problem hiding this comment.
Does Next add $schema and observability?
It adds the $schema but not observability:
https://github.com/opennextjs/opennextjs-cloudflare/blob/6727373c31600b627e3c633b9bc4d24013927904/packages/cloudflare/templates/wrangler.jsonc#L2
That's my bad I can update it to also enable observability
There was a problem hiding this comment.
Updated to combine wrangler.json(c) files generated by the framework with our own: fe581f5
This should solve all the concerns both for Next.js and any other framework that can generate a wrangler config file
PS: for simplicity, wrangler.toml files are not supported here, also potential comments in wrangler.jsonc files are not preserved, I could add logic to also handle such cases if we want to (but that might be a bit involved, and potentially not really needed)
|
Claude finished @dario-piotrowicz's task —— View job Changeset ReviewReviewing changeset files for this PR...
The changeset 1. Missing Body/ExplanationThe changeset only has a title but no body explaining the change. According to the guidelines:
Recommendation: Add a body that explains:
Example improvement: ---
"wrangler": patch
---
Update autoconfig logic to handle Next.js projects by using the new `@opennextjs/cloudflare migrate` command
The autoconfig flow for Next.js projects now delegates to `@opennextjs/cloudflare migrate` instead of maintaining custom OpenNext configuration logic. This ensures compatibility with the latest OpenNext tooling and reduces maintenance burden. The wrangler configuration generated by the migrate command is now preserved and merged with autoconfig's settings rather than being overwritten.2. Version Type is AcceptableThe |
…gler config file
dario-piotrowicz
force-pushed
the
dario/DEVX-2383/opennext-init
branch
from
96b2d80 to
fe581f5
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
4 participants
Fixes https://jira.cfdata.org/browse/DEVX-2383
This PR removes the OpenNext logic we have in the wrangler autoconfig flow in favour of using the newly introduced
migratecommand from@opennextjs/cloudflaremigratecommand and previewed the application locallyA picture of a cute animal (not mandatory, but encouraged)