Conversation
…fig file (required by open-next) if it doesn't exist
🦋 Changeset detectedLatest commit: 6d0343a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
| "Error: This does not appear to be a Next.js application.\n" + | ||
| "The 'next' package is not installed and no next.config file was found." | ||
| ); | ||
| process.exit(1); |
There was a problem hiding this comment.
ensureNextConfigExists is a pretty generic name. I would throw here and let the caller decide if they should log the error and/or process.exit. WDYT.
There was a problem hiding this comment.
Rgardingprocess.exit, I put it here as that seemed to me the general pattern in the open-next codebase, I'm happy to throw instead and have the caller exit 👍
There was a problem hiding this comment.
ensureNextConfigExists is a pretty generic name
I've updated the name to createNextConfigFileIfMissing I hope this is better?
There was a problem hiding this comment.
In regards to process.exit, what do you think about
75229f9
?
| process.exit(1); | ||
| } | ||
|
|
||
| printStepTitle("Creating next.config.ts"); |
There was a problem hiding this comment.
Again I don't think think should be the (single) responsibility of ensureNextConfigExists to do that ?
There was a problem hiding this comment.
I've renamed the function createNextConfigFileIfMissing and removed the printStepTitle, does this work for you?
Co-authored-by: Victor Berchet <victor@suumit.com>
Co-authored-by: Victor Berchet <victor@suumit.com>
vicb
left a comment
There was a problem hiding this comment.
Sorry quick review but at least some of the comment should be meaningful
vicb
left a comment
There was a problem hiding this comment.
Looks great, I think you need to update the usage of ensureNextjsVersionSupported ?
| } | ||
| ); | ||
| } | ||
| // At this point we're sure the next config exists |
There was a problem hiding this comment.
super nit: we are sure that it has been created... not that it (still) exists. Not asking any change here.
There was a problem hiding this comment.
ok 😄
Anyways I've improved this a bit: 23ea438, is this better? 🙂
| if (!skipNextVersionCheck && !ensureNextjsVersionSupported({ nextVersion })) { | ||
| throw new Error(`Next.js version ${nextVersion} is not compatible with OpenNext.`); | ||
| } |
There was a problem hiding this comment.
Shouldn't this be just:
| if (!skipNextVersionCheck && !ensureNextjsVersionSupported({ nextVersion })) { | |
| throw new Error(`Next.js version ${nextVersion} is not compatible with OpenNext.`); | |
| } | |
| if (!skipNextVersionCheck) { | |
| ensureNextjsVersionSupported({ nextVersion }); | |
| } |
There was a problem hiding this comment.
of course! 🤦
opennextjs/opennextjs-cloudflare#1127 Changeset: .changeset/port-pr-1127.md
* Add SKILL.md for porting PRs and AGENTS.md for coding guidelines * update skill * Port opennextjs/opennextjs-aws#1118 as a test * Port opennextjs/opennextjs-aws#1117 * update skill * Port opennextjs/opennextjs-aws#1114 * Port PR opennextjs/opennextjs-aws#1107 * update skills * Port PR opennextjs/opennextjs-aws#1108 * Port PR opennextjs/opennextjs-aws#1104 * Port PR opennextjs/opennextjs-aws#1101 * Port PR opennextjs/opennextjs-aws#1098 * chore: port PR #1083 from source repository opennextjs/opennextjs-cloudflare#1083 Changeset: .changeset/port-pr-1083.md * chore: port PR #1105 from source repository opennextjs/opennextjs-cloudflare#1105 Changeset: .changeset/port-pr-1105.md * chore: port PR #1097 from source repository opennextjs/opennextjs-cloudflare#1097 Changeset: .changeset/port-pr-1097.md * chore: port PR #1122 from source repository opennextjs/opennextjs-cloudflare#1122 Applied bugfixes and improvements to the 'migrate' command: - Fixed extra newlines when appending to files (updated conditionalAppendFileSync signature) - Fixed error when 'public' directory is missing (creates parent directories automatically) - Fixed Next.js config file update to check if the file exists first - Updated checkRunningInsideNextjsApp to accept { appPath: string } instead of full BuildOptions Changesets: - .changeset/port-pr-1122-cloudflare.md - .changeset/port-pr-1122-aws.md * chore: update port PR skill instructions for staging and committing changes * chore: port PR #1126 from source repository opennextjs/opennextjs-cloudflare#1126 Fix: prevent Worker hang on HEAD requests to static assets When run_worker_first is enabled, HEAD requests to static assets hang the Worker because response.body is null (per HTTP spec) and the fallback new ReadableStream() creates a stream that never closes. Changes: - Return null body for HEAD requests instead of falling through to the hanging ReadableStream fallback - Add tests for maybeGetAssetResult covering GET, HEAD, 404, POST, and run_worker_first=false cases Changeset: .changeset/port-pr-1126.md * linting * chore: port PR #1127 from source repository opennextjs/opennextjs-cloudflare#1127 Changeset: .changeset/port-pr-1127.md * chore: port PR #1138 from source repository opennextjs/opennextjs-cloudflare#1138 Changeset: .changeset/port-pr-1138.md * chore: port PR #1133 from source repository opennextjs/opennextjs-cloudflare#1133 Changeset: .changeset/port-pr-1133.md Update the migrate command to attempt to create an R2 bucket for caching as part of the migration process, if that is not possible an application without caching enabled will be generated instead. * chore: port PR #1142 from source repository opennextjs/opennextjs-cloudflare#1142 Changeset: .changeset/port-pr-1142.md * chore: port PR #1147 from source repository opennextjs/opennextjs-cloudflare#1147 make dev /cdn-cgi/image behaves like prod for consistency Changeset: .changeset/port-pr-1147.md * chore: port PR #1150 from source repository opennextjs/opennextjs-cloudflare#1150 Changeset: .changeset/port-pr-1150.md * fix lockfile * fix test
Fixes https://jira.cfdata.org/browse/DEVX-2461
The next config file is generally present in Next.js apps but it's not actually mandatory (for example see: https://github.com/vercel/examples/tree/main/solutions/blog).
In any case open-next does require the file (see: opennextjs/opennextjs-aws#1111), so the changes here improve the
migratecommand to create this file if not already present (enabling a migration for projects that don't include the file).