Use Node 24.9.0 across the whole repository #1559
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRepository-wide Node.js upgrade pinned to 24.9.0. Changes: Docker base images updated to node:24.9.0-alpine* in several Dockerfiles; many package.json files now include Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (21)
packages/cyberstorm-forms/package.json (1)
17-19: Engine pin looks good; decide policy: exact vs range.If you want security patches without PR churn, consider >=24.9.0 <25 or ^24.9.0; if you want strict reproducibility, keep exact but ensure CI/dev tooling enforces it. Your call—just be consistent across the workspace.
packages/use-promise/package.json (1)
16-18: Consistent Node requirement—LGTM.Same note as others: either keep exact pin or switch to >=24.9.0 <25. Stay consistent across all packages.
packages/ts-uploader-react/package.json (1)
16-18: Engines alignment ok; consider adding package manager pinning at root.Adding "packageManager" (e.g., "pnpm@x"/"yarn@x"/"npm@x") at the repo root helps avoid mismatches with Node 24.9.0 tooling locally and in CI.
package.json (2)
14-16: Optional: declare package manager and local version manager files.Add "packageManager" at root and commit .node-version/.nvmrc set to 24.9.0 to tighten local dev parity with CI.
27-27: Ensure workspace-level dedupe for @types/node ^24
To avoid mixed Node type versions, add a workspace override/resolution for "@types/node": "^24" (e.g. Yarn’sresolutionsor npm’soverrides), or ensure each package that declares it also targets ^24.
Optional check:#!/bin/bash # List manifests pinning @types/node to versions other than ^24 grep -RP '"@types/node"\s*:\s*"(?!\^24)[^"]+"' -n --exclude-dir=node_modules . || echo "All good (no mismatches found)."apps/cyberstorm-remix/Dockerfile.development (1)
2-2: Base image bump looks good; make Yarn availability explicit.Recommend enabling Corepack (and optionally pinning Yarn) before install to avoid image drift:
# before RUN ... yarn install RUN corepack enable # optionally: RUN corepack prepare yarn@<your-version> --activateapps/cyberstorm-remix/package.json (1)
16-18: App pin LGTM; also pin the package manager for reproducible installs.Keep the exact Node pin here. Add a packageManager field (e.g., "yarn@") and commit an .nvmrc/Volta config so local/CI stay aligned with Corepack.
packages/ts-api-react-forms/package.json (1)
16-18: Loosen engines.node for a library to avoid blocking consumers.Use a broader range (e.g., ">=18 <25" or ">=24.9.0 <25") unless you explicitly want to enforce Node 24.9.0 for consumers. Keep strict pins only for apps.
packages/dapper-fake/package.json (1)
16-18: Loosen strict Node engine pin to a semver range for published libraries
All internal packages and Dockerfiles currently pinnodeto 24.9.0. For library packages, change your package.json to"engines": { "node": ">=24.9.0 <25" }—and keep the exact pin only in your standalone apps or CI Docker images. Add a
.nvmrcor Volta config to lock local dev to 24.9.x.packages/react-dnd/package.json (1)
16-18: Loosen Node engine version in package.json
In packages/react-dnd/package.json (lines 16–18) you pin Node to “24.9.0”. For published libraries, use a range (e.g. “>=18 <25” or “>=24.9.0 <25”) to avoid excluding consumers on other LTS lines. Several other public packages here also use an exact pin—consider updating engines.node across them.packages/thunderstore-api/package.json (1)
24-26: Pin accepted; consider range if patch bumps should auto-apply.If you want CI to accept future 24.x patches, consider ">=24.9.0 <25". If strict pinning is intentional, ignore.
tools/cyberstorm-playwright/package.json (1)
14-14: Node types bump looks good; consider adding engines for consistency.Since the PR’s goal is repo‑wide standardization, add an engines.node to this tools package too, or confirm it inherits enforcement from the workspace root.
"devDependencies": { "@percy/cli": "^1.29.2", "@percy/playwright": "^1.0.6", "@playwright/test": "^1.46.1", "@types/node": "^24" - } + }, + "engines": { + "node": "24.9.0" + }packages/cyberstorm/package.json (2)
17-19: Prefer a range over an exact Node version.Keep consumers on 24.x without forcing a single patch.
- "engines": { - "node": "24.9.0" - }, + "engines": { + "node": ">=24.9.0 <25" + },
40-40: Move @types/node to devDependencies.Types shouldn’t be a runtime dependency for this UI library.
"dependencies": { @@ - "@types/node": "^24", "@types/react": "^19.1.0", @@ }, "devDependencies": { + "@types/node": "^24", "typescript": "^5.6.2", "typescript-plugin-css-modules": "^5.1.0" }apps/cyberstorm-storybook/package.json (1)
12-14: Use a 24.x engines range instead of an exact pin.Prevents needless bumps for every Node patch.
- "engines": { - "node": "24.9.0" - }, + "engines": { + "node": ">=24.9.0 <25" + },packages/dapper-ts/package.json (1)
19-21: Loosen Node engine range in all package.json files
18 packages currently pin"node": "24.9.0". Update each to:- "engines": { - "node": "24.9.0" - }, + "engines": { + "node": ">=24.9.0 <25" + },This allows any Node 24.x release. You can automate with:
find . -type f -name package.json -exec \ jq 'if .engines.node == "24.9.0" then .engines.node = ">=24.9.0 <25" else . end' -i {} \;.github/workflows/auto-merge-dependabot.yml (1)
11-15: Nice bump to checkout@v5; consider hardening this job.
- You likely don’t need a checkout here; the auto‑merge action works without it. Removing it reduces risk and speeds up the job.
- If you keep checkout, disable token persistence and shallow fetch.
- Add minimal permissions and restrict to Dependabot actor; pin third‑party action to a commit SHA.
Proposed diff:
steps: - - uses: actions/checkout@v5 + # Checkout not required for auto-merge; remove to reduce risk + # - uses: actions/checkout@v5 + # with: + # persist-credentials: false + # fetch-depth: 1 - uses: ahmadnassri/action-dependabot-auto-merge@v2 with: target: minor github-token: ${{ secrets.AUTO_MERGE_GITHUB_PAT }}And recommended job hardening (add near
runs-on):auto-merge: name: Auto-merge runs-on: ubuntu-latest + if: github.actor == 'dependabot[bot]' + permissions: + contents: write + pull-requests: write.github/actions/get-node-version/package.json (1)
10-16: Engine pin looks consistent; tighten deps classification.
- Keep the engine pin if that’s the strategy repo‑wide.
- Move TypeScript tooling to devDependencies to avoid bloating action runtime layers.
Suggested diff:
"dependencies": { - "@actions/core": "^1.4.0", - "@types/node": "^24", - "@types/semver": "^7.3.8", - "semver": "^7.3.5", - "typescript": "^5.6.2" + "@actions/core": "^1.4.0", + "semver": "^7.3.5" }, + "devDependencies": { + "@types/node": "^24", + "@types/semver": "^7.3.8", + "typescript": "^5.6.2" + }If you’d prefer patch flexibility while staying on 24: consider
"node": ">=24.9.0 <25"instead of an exact pin. Confirm intent.apps/cyberstorm-storybook/Dockerfile (1)
4-12: Base image bump LGTM; consider pinning Yarn and trimming image.
- Enable Corepack and pin Yarn to avoid accidental upgrades at build time.
- Set NODE_ENV=production for smaller installs, and clear Yarn cache.
Proposed additions:
-FROM node:24.9.0-alpine3.21 AS builder +FROM node:24.9.0-alpine3.21 AS builder +ENV NODE_ENV=production +RUN corepack enable && corepack prepare yarn@stable --activate @@ -RUN yarn install --frozen-lockfile +RUN yarn install --frozen-lockfile && yarn cache clean.github/actions/get-node-version/Dockerfile (1)
2-8: Runtime aligned with Node 24.9.0; add Corepack/Yarn pin and prune dev bits.
- Enable Corepack and pin Yarn to stabilize builds.
- If the build emits a self-contained dist, install production only and clean cache to slim the image.
Example:
-FROM node:24.9.0-alpine +FROM node:24.9.0-alpine WORKDIR /src +RUN corepack enable && corepack prepare yarn@stable --activate COPY package.json yarn.lock ./ -RUN yarn install --frozen-lockfile +RUN yarn install --frozen-lockfile --production=false COPY . . RUN yarn build +RUN yarn workspaces focus --production || true +RUN yarn cache clean CMD ["node", "/src/dist/main.js"].github/workflows/visual-diff.yml (1)
19-41: Update CI with minimal permissions, action pinning, and fixed repo-wide checks
Add minimal job-level permissions:
permissions: contents: readPin all third-party actions (e.g.
snok/install-poetry@<commit-SHA>).Repo-wide verification script for leftover Node versions:
#!/usr/bin/env bash set -euo pipefail echo "Dockerfiles not on 24.9.0:" rg -g '**/Dockerfile' -n 'FROM[[:space:]]node:' | rg -v 'node:24\.9\.0' || true echo -e "\npackage.json engines not at 24.9.0:" rg -g '**/package.json' -n '"node":' | rg -v '"node":[[:space:]]*"24\.9\.0"' || true echo -e "\nactions/setup-node uses without get-node-version:" rg -g '.github/workflows/*.yml' -n 'uses:.*actions/setup-node@v5' -C2 \ | rg -v 'node-version:[[:space:]]*\$\{\{ *needs\.get-node-version\.outputs\.node-version' || true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/actions/get-node-version/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
.github/actions/get-node-version/Dockerfile(1 hunks).github/actions/get-node-version/package.json(1 hunks).github/actions/get-node-version/src/main.ts(3 hunks).github/workflows/auto-merge-dependabot.yml(1 hunks).github/workflows/static-analysis.yml(3 hunks).github/workflows/test.yml(6 hunks).github/workflows/visual-diff.yml(2 hunks)apps/cyberstorm-remix/Dockerfile(1 hunks)apps/cyberstorm-remix/Dockerfile.development(1 hunks)apps/cyberstorm-remix/package.json(1 hunks)apps/cyberstorm-storybook/Dockerfile(1 hunks)apps/cyberstorm-storybook/package.json(1 hunks)package.json(2 hunks)packages/cyberstorm-forms/package.json(1 hunks)packages/cyberstorm/package.json(2 hunks)packages/dapper-fake/package.json(1 hunks)packages/dapper-ts/package.json(1 hunks)packages/dapper/package.json(1 hunks)packages/graph-system/package.json(1 hunks)packages/react-dnd/package.json(1 hunks)packages/thunderstore-api/package.json(1 hunks)packages/ts-api-react-actions/package.json(1 hunks)packages/ts-api-react-forms/package.json(1 hunks)packages/ts-api-react/package.json(1 hunks)packages/ts-uploader-react/package.json(1 hunks)packages/ts-uploader/package.json(1 hunks)packages/typed-event-emitter/package.json(1 hunks)packages/use-promise/package.json(1 hunks)tools/cyberstorm-playwright/package.json(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/test.yml
37-37: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
42-42: property "get-node-version" is not defined in object type {setup-python: {conclusion: string; outcome: string; outputs: {string => string}}}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Generate visual diffs
🔇 Additional comments (10)
packages/dapper/package.json (1)
28-30: Match workspace policy and enforce in tooling.Good alignment. Ensure the package manager/CI actually enforces engines (e.g., corepack + setup-node uses 24.9.0, engine‑strict as needed).
package.json (1)
15-16: Ensure consistent Node version across all workspacesUse this updated script (runs under Bash and uses POSIX tests):
#!/usr/bin/env bash set -euo pipefail EXPECT="24.9.0" find . \ \( -path "./apps/*/package.json" -o -path "./packages/*/package.json" -o -name "package.json" \) \ -print0 | while IFS= read -r -d '' f; do val=$(jq -r '.engines.node // empty' "$f") if [ -z "$val" ]; then echo "MISSING engines.node -> $f" elif [ "$val" != "$EXPECT" ]; then echo "MISMATCH ($val) -> $f" fi doneRun it with
bash ./check-node.shfrom the repo root to catch any missing or mismatched engines.node entries.packages/ts-api-react-actions/package.json (1)
16-18: Consistent with the repo-wide target.No issues from me.
packages/ts-uploader/package.json (1)
18-20: Looks good.Aligned with Node 24.9.0; no further changes needed here.
apps/cyberstorm-storybook/package.json (1)
7-11: No action needed for Chromatic script
Chromatic is declared in the root package.json and Yarn v1 workspaces hoist binaries into all workspaces, so thechromaticscript in apps/cyberstorm-storybook will resolve correctly..github/workflows/static-analysis.yml (1)
32-32: LGTM! Action version updates are correct.The upgrades to actions/checkout@v5 and actions/setup-node@v5 align with the PR's modernization of workflow tooling and are compatible with the dynamic node-version sourcing from get-node-version outputs.
Also applies to: 47-49, 81-81
.github/actions/get-node-version/src/main.ts (1)
1-1: LGTM! Good observability enhancement.Adding info logging for validation and output-setting improves transparency without altering behavior. This is a solid best practice for GitHub Actions.
Also applies to: 14-14, 52-52
apps/cyberstorm-remix/Dockerfile (1)
2-2: LGTM! Base image updated correctly.The Node.js version bump to 24.9.0 aligns with the PR's standardization objective.
.github/workflows/test.yml (2)
19-19: LGTM! Checkout action upgraded correctly.The update to actions/checkout@v5 is consistent across all jobs.
Also applies to: 32-32, 98-98, 169-169
99-102: LGTM! Node version sourcing is correct.The
testandchromatic-deploymentjobs properly declareneeds: get-node-versionand reference the outputs correctly.Also applies to: 172-176
Oksamies
left a comment
Oksamies
left a comment
There was a problem hiding this comment.
Good changes 👍 IIRC there are bunch of good features in node 24 regarding Typescript configs and linting, so that could be an additional task.
anttimaki
force-pushed
the
one-node-to-rule-them-all
branch
from
a591821 to
af05bea
Compare
|
Conflicts and additional changes needed because of latest merges. Let me know if you want me to do those, as might be inefficiently used time if you need to go through the whole project again. |
- Use v24.9.0, disallowing automatic updates to ensure all envs are identical in this matter - Add the engine field for all packages as it's not inherited from the root package - Update @types/node for packages currently using it
Docker for get-node-version doesn't depend on the exact same Alpine release since it might make running the GitHub actions brittle as we can't control the available versions.
The latest v5 targets the Node v24 we are using in the project.
- Use output of get-node-version job to determine node version - Update actions/setup-python to recent version as v2 is EOLd
anttimaki
force-pushed
the
one-node-to-rule-them-all
branch
from
af05bea to
6ae7a9d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1559 +/- ##
=======================================
Coverage 10.70% 10.70%
=======================================
Files 285 285
Lines 19871 19871
Branches 385 385
=======================================
Hits 2128 2128
Misses 17743 17743 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: anttimaki
PR: thunderstore-io/thunderstore-ui#1559
File: packages/ts-api-react/package.json:16-18
Timestamp: 2025-10-01T11:36:11.065Z
Learning: In the thunderstore-ui repository, exact Node version pinning (e.g., "24.9.0") is intentionally used in package.json engines fields rather than semver ranges, to ensure consistency across environments.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
| needs: get-node-version | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/checkout@v5 | ||
| with: | ||
| submodules: false | ||
| - name: Setup Python 3.8 | ||
| id: setup-python | ||
| uses: actions/setup-python@v2 | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.8" | ||
| - uses: actions/setup-node@v4 | ||
| - uses: actions/setup-node@v5 | ||
| with: | ||
| node-version: "24" | ||
| - name: Get node version | ||
| id: node-version | ||
| run: echo "::set-output name=node-version::$(node --version)" | ||
| node-version: ${{ needs.get-node-version.outputs.node-version }} | ||
| - uses: snok/install-poetry@v1 |
There was a problem hiding this comment.
Critical: new dependency skips CI on same‑repo PRs
By adding needs: get-node-version here while the upstream job still has if: github.event_name != 'pull_request' || …, every internal PR (base repo id == head repo id) will now skip get-node-version, so this job — and every other job that declares the same dependency — gets skipped as well. Net result: CI no longer runs for any branch PR created inside this repository. Please either drop/relax the if on the get-node-version job or make this job resilient when that job is skipped.
get-node-version:
name: Get Node version
- if: github.event_name != 'pull_request' || (github.event_name == 'pull_request' && github.event.base.repo.id != github.event.head.repo.id)
+ # Allow internal PRs to run; the dependency chain now requires this job every time.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| needs: get-node-version | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v5 | |
| with: | |
| submodules: false | |
| - name: Setup Python 3.8 | |
| id: setup-python | |
| uses: actions/setup-python@v2 | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.8" | |
| - uses: actions/setup-node@v4 | |
| - uses: actions/setup-node@v5 | |
| with: | |
| node-version: "24" | |
| - name: Get node version | |
| id: node-version | |
| run: echo "::set-output name=node-version::$(node --version)" | |
| node-version: ${{ needs.get-node-version.outputs.node-version }} | |
| - uses: snok/install-poetry@v1 | |
| get-node-version: | |
| name: Get Node version | |
| # Allow internal PRs to run; the dependency chain now requires this job every time. |
🤖 Prompt for AI Agents
.github/workflows/test.yml lines 31-44: the job currently declares needs:
get-node-version which causes the entire job to be skipped for same-repo PRs
because the get-node-version job is itself gated by an if that skips for PRs;
fix by either (A) removing or relaxing the restrictive if on the
get-node-version job so it always runs for PRs, or (B) make this job resilient
by removing the hard dependency and instead conditionally use the
get-node-version output (e.g., only reference
needs.get-node-version.outputs.node-version when needs.get-node-version.outcome
== 'success' or fallback to a default node-version), ensuring the job can run
even when get-node-version was skipped.
|
@Oksamies I think I got the conflict resolution right and handled the new parts to |
No description provided.