Skip to content
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

bump C3's create-next-app to 15.0.3 #7241

Merged
merged 8 commits into from
Dec 12, 2024
Merged

bump C3's create-next-app to 15.0.3 #7241

merged 8 commits into from
Dec 12, 2024

Conversation

dario-piotrowicz
Copy link
Member

@dario-piotrowicz dario-piotrowicz commented Nov 12, 2024

This PR is for updating C3 to create a Next.js 15 app in non-experimental mode

However (see comments below) the bump of create-next-app for non-experimental mode currently applies to both the non-experimental and experimental Next.js templates, forcing us to either make sure that both templates can work with Next.js 15 or that we find a way to have each use a different version of create-next-app


  • Tests
    • TODO (before merge)
    • Tests included (the quarantined e2e Next.js test has been updated)
    • Tests not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because: I think that the Next.js (non-experimental) e2e test isn't run because it is quarantined
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: it's just a version bump

Copy link
Contributor

github-actions bot commented Nov 12, 2024

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12282270102/npm-package-wrangler-7241

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7241/npm-package-wrangler-7241

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12282270102/npm-package-wrangler-7241 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12282270102/npm-package-create-cloudflare-7241 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12282270102/npm-package-cloudflare-kv-asset-handler-7241
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12282270102/npm-package-miniflare-7241
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12282270102/npm-package-cloudflare-pages-shared-7241
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12282270102/npm-package-cloudflare-vitest-pool-workers-7241
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12282270102/npm-package-cloudflare-workers-editor-shared-7241
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12282270102/npm-package-cloudflare-workers-shared-7241
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12282270102/npm-package-cloudflare-workflows-shared-7241

Note that these links will no longer work once the GitHub Actions artifact expires.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241205.0
workerd 1.20241205.0 1.20241205.0
workerd --version 1.20241205.0 2024-12-05

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

Copy link

changeset-bot bot commented Dec 8, 2024

🦋 Changeset detected

Latest commit: 8dfcae6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
create-cloudflare Patch

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

@dario-piotrowicz
Copy link
Member Author

Note

This PR updates the non-experimental Next.js C3 template, so I think that the C3 E2E Tests (experimental) checks (which are failing because of Next.js) can safely be ignored (I can deal with them in a followup PR if that's ok?)

Copy link
Contributor

@andyjessop andyjessop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @dario-piotrowicz

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I think that the C3 E2E Tests (experimental) checks (which are failing because of Next.js) can safely be ignored

I don't think it is fair to break the experimental Next.js support by bumping the Next.js dependency.
If we can't separate them to use different versions of Next.js then I think we need to ensure they are both fixed together.

@dario-piotrowicz
Copy link
Member Author

so I think that the C3 E2E Tests (experimental) checks (which are failing because of Next.js) can safely be ignored

I don't think it is fair to break the experimental Next.js support by bumping the Next.js dependency. If we can't separate them to use different versions of Next.js then I think we need to ensure they are both fixed together.

ah yeah for sure! I was under the assumption that the experimental e2e was failing either way I didn't realize the bump was breaking it (but it clearly runs without the bump: example run)

it's quite unfortunate that the create-next-app bump effects both templates 😕 😓

@petebacondarwin
Copy link
Contributor

I think @vicb was working up updating opennextjs?

@dario-piotrowicz
Copy link
Member Author

I'm looking at the code and I don't think there's a clean way to use different versions of create-next-app 😓

We could have a new package.json just for the experimental templates... that feels like the cleanest solution, but also very much overkill for just Next.js 😓

Alternatively we could have some very hacky if statement that overrides the create-next-app version for the experimental Next.js template...

Or yeah just wait for our opennext adapter to support Next 15 😓

// we do this because the stable @opennextjs/cloudflare package
// does not yet support Next 15, we should move away from this
// as soon as possible
"https://pkg.pr.new/@opennextjs/cloudflare@experimental",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at the details but the experimental branch should have an open next config file and a different wrangler.toml.
Should they be updated as part of the PR?

Also loading instrumentation will fall on 15.0.3 if not patched.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah of course, good callout 😓👍

@dario-piotrowicz
Copy link
Member Author

dario-piotrowicz commented Dec 11, 2024

@petebacondarwin what do you think of this temporary workaround for the experimental template issue?

d2bd8b1

* The version of the framework cli tool to use.
* If omitted the cli version is taken from src/frameworks/package.json, which is the default/standard behavior.
*/
frameworkCliVersion?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be to allow frameworkCli to specify the version (i.e. [email protected])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is fine. You could even change it to something to make it clear this is exceptional:

Suggested change
frameworkCliVersion?: string;
pinFrameworkCliVersion?: string;

We use dependabot to keep the CLI versions up to date, which is why they are stored in a package.json. Moving the versions here would mess that up. But in this case we don't want dependabot to do anything with this version since we are explictly pinning it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposal is to use either cli to use the version from the package.json or cli@version to pin the version as an alternative to using the pinFrameworkCliVersion property

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I understand. But in doing that it is not clear that this is not the intended normal way to specify the version in cases where we do not need to pin the version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petebacondarwin I've renamed the field as you suggested 🙂

I've also updated the comment to make the thing a bit clearer

c36670d

* The version of the framework cli tool to use.
* If omitted the cli version is taken from src/frameworks/package.json, which is the default/standard behavior.
*/
frameworkCliVersion?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is fine. You could even change it to something to make it clear this is exceptional:

Suggested change
frameworkCliVersion?: string;
pinFrameworkCliVersion?: string;

We use dependabot to keep the CLI versions up to date, which is why they are stored in a package.json. Moving the versions here would mess that up. But in this case we don't want dependabot to do anything with this version since we are explictly pinning it.

@dario-piotrowicz dario-piotrowicz merged commit d8e96b2 into main Dec 12, 2024
30 checks passed
@dario-piotrowicz dario-piotrowicz deleted the dario/c3-next-15 branch December 12, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants