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 <[email protected]>
Co-authored-by: Victor Berchet <[email protected]>
vicb
left a comment
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
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! 🤦
2 participants
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).