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 node.js to v.20 for all C3 workflows #7648

Closed
wants to merge 1 commit into from

Conversation

dario-piotrowicz
Copy link
Member

The C3 e2es (both experimental and not) tests for Nuxt using yarn under ubuntu seem to be consistently failing on every PR that runs them.

By looking at the logs it looks like Nuxt is incompatible with the version of node that we're using in our workflows:
Screenshot 2024-12-30 at 11 53 44

So in this PR I am simply bumping the node version used by all the C3 workflows as that should be pretty safe to do I think.

I did not change the default node version for all workflows as I assume that we have a specific reason as to why we want to use 18.20.2 there (if not I am happy to change that value instead)


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because: this is just a CI change
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: this is just a CI change

@dario-piotrowicz dario-piotrowicz added the e2e Run wrangler e2e tests on a PR label Dec 30, 2024
@dario-piotrowicz dario-piotrowicz requested a review from a team as a code owner December 30, 2024 11:00
Copy link

changeset-bot bot commented Dec 30, 2024

⚠️ No Changeset found

Latest commit: 9799b6d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

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/12545621975/npm-package-wrangler-7648

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-wrangler-7648 dev path/to/script.js
Additional artifacts:
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-cloudflare-workers-bindings-extension-7648 -O ./cloudflare-workers-bindings-extension.0.0.0-v0d6ce2f46.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v0d6ce2f46.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-create-cloudflare-7648 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-cloudflare-kv-asset-handler-7648
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-miniflare-7648
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-cloudflare-pages-shared-7648
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-cloudflare-unenv-preset-7648
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-cloudflare-vitest-pool-workers-7648
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-cloudflare-workers-editor-shared-7648
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-cloudflare-workers-shared-7648
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12545621975/npm-package-cloudflare-workflows-shared-7648

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.20241218.0
workerd 1.20241218.0 1.20241218.0
workerd --version 1.20241218.0 2024-12-18

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

@edmundhung
Copy link
Member

edmundhung commented Dec 30, 2024

Wrangler v3 currently supports Node v16+ as stated in the docs. But we are changing it to 18+ in v4 following the Nodejs support window (https://nodejs.org/en/about/previous-releases). So the wrangler workflow should probably stay in node16 for now.

But if we are just changing the c3 tests for frameworks to use node 18 20, i think it is fine?

@dario-piotrowicz
Copy link
Member Author

Wrangler v3 currently supports Node v16+ as stated in the docs. But we are changing it to 18+ in v4 following the Nodejs support window (https://nodejs.org/en/about/previous-releases). So the wrangler workflow should probably stay in node16 for now.

The value I was referring to is the default one for the install-dependecies action:

So, I think that it should be the one most commonly used across all the various GitHub workflows.

There are a few gh workflows that use the action: GitHub search, I am not too involved/familiar with the monorepo so I cannot really say what default would make most sense without digging deeper into things a bit.

Interestingly the wrangler e2es are already purposely run under node 18 actually (instead of 16):

But if we are just changing the c3 tests for frameworks to use node 18, i think it is fine?

yeah I do think so, alternatively we skip the nuxt+yarn+ubuntu tests for the time being, but I would personally prefer to just bump the node version as I think we can reasonably expect people to use a relatively newer version of node when scaffolding a new application.

Actually 20 is not an LTS, so I wonder if it would be better to just straight jump to 22 🤔
Screenshot 2024-12-30 at 14 25 16

One thing to note is that c3 has support for node 18 and up:

Anyways I am fine with whatever the team prefers 🙂

@edmundhung
Copy link
Member

One thing to note is that c3 has support for node 18 and up:

In that case I think c3 workflow should use node 18 in general, but node 20+ just for framework tests. We have no control over the node version requirements of the frameworks but the rest of the template should remain supported with node 18.

@dario-piotrowicz
Copy link
Member Author

In that case I think c3 workflow should use node 18 in general, but node 20+ just for framework tests. We have no control over the node version requirements of the frameworks but the rest of the template should remain supported with node 18.

This makes sense to me 👍 , unfortunately C3 e2es run all together, we don't currently have a way to run separately the ones that use frameworks from the ones that don't 🥲

Of course we could implement that, but it would, in my opinion, require quite some refactoring and make the e2es process generally more complicated, so I don't think that it would be worth it 😕

Personally I would go with just using node 20+ (or 22+ as I mentioned) and:

  • as I mentioned assume that people do use modern node versions for new apps scaffolding
  • assume that if things work fine with node 20+/22+ then most likely they run well with node 18+ (since we don't use any newer fancy node feature as far as I know)
  • on the next major version of C3 we bump the node requirement accordingly realigning everything

@dario-piotrowicz
Copy link
Member Author

Closing in favour of #7649 (turns out a non-controversial patch bump was enough 🤦)

@dario-piotrowicz dario-piotrowicz deleted the dario/c3-workflows-node-bump branch December 30, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run wrangler e2e tests on a PR no-changeset-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants