Migrate project from npm to pnpm v10 with CI updates#3351
Draft
wozniakpl wants to merge 11 commits intoopenfrontio:mainfrom
Draft
Migrate project from npm to pnpm v10 with CI updates#3351wozniakpl wants to merge 11 commits intoopenfrontio:mainfrom
wozniakpl wants to merge 11 commits intoopenfrontio:mainfrom
Conversation
- Add packageManager and pnpm lockfile - Require pnpm v10 in README and CONTRIBUTING - CI: use pnpm/action-setup with version 10
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMigrates project from npm/npx to pnpm across CI, Docker, docs, VS Code launch, supervisord, and package scripts; updates one lit import path in client code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
- Updated the CI configuration to remove the explicit version setting for pnpm in the setup steps, streamlining the workflow.
- Updated Dockerfile to use pnpm for dependency installation and build commands. - Modified package.json scripts to utilize pnpm for performance improvements. - Adjusted README and supervisord.conf to reflect the transition to pnpm.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
124-128:⚠️ Potential issue | 🟡 MinorChange lint command to pnpm for consistency.
Line 127 in README.md uses
npm run lintwhile all other commands use pnpm. Update it to match the project standard.Suggested patch
- npm run lint + pnpm run lint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 124 - 128, Replace the npm lint command in the README (the line containing "npm run lint") with the project-standard pnpm invocation (e.g., "pnpm run lint" or "pnpm lint") so the lint entry matches other pnpm commands in the file; update the exact string "npm run lint" to "pnpm run lint" within README.md.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
23-26: Pin pnpm version in CI to matchpackage.json.Using
version: 10in the workflow whilepackage.jsonpinspnpm@10.30.3creates unnecessary differences between local and CI environments. Pin the exact version in all fourpnpm/action-setup@v4blocks.Suggested patch
- version: 10 + version: 10.30.3(Apply to all four instances: lines 26, 50, 67, and 83.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 23 - 26, The CI workflow uses pnpm/action-setup@v4 with a generic version: 10; update each "Setup pnpm" step (the actions that use "pnpm/action-setup@v4") to pin the exact pnpm version matching package.json (pnpm@10.30.3) by replacing the with: version: 10 entries in all four blocks with with: version: 10.30.3 so CI matches local dev; ensure you update every instance of the pnpm/action-setup@v4 step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@README.md`:
- Around line 124-128: Replace the npm lint command in the README (the line
containing "npm run lint") with the project-standard pnpm invocation (e.g.,
"pnpm run lint" or "pnpm lint") so the lint entry matches other pnpm commands in
the file; update the exact string "npm run lint" to "pnpm run lint" within
README.md.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 23-26: The CI workflow uses pnpm/action-setup@v4 with a generic
version: 10; update each "Setup pnpm" step (the actions that use
"pnpm/action-setup@v4") to pin the exact pnpm version matching package.json
(pnpm@10.30.3) by replacing the with: version: 10 entries in all four blocks
with with: version: 10.30.3 so CI matches local dev; ensure you update every
instance of the pnpm/action-setup@v4 step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd63e5a8-8cf9-4651-95f3-9b0fa4aee559
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.github/workflows/ci.yml.vscode/launch.jsonCONTRIBUTING.mdREADME.mdpackage.jsonsrc/client/graphics/layers/PlayerInfoOverlay.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
36-47:⚠️ Potential issue | 🔴 CriticalMissing corepack enable in final stage.
The final production image needs pnpm for
supervisord.confto runpnpm run start:server. Addcorepack enableafter installing system dependencies.🐛 Proposed fix
RUN apt-get update && apt-get install -y \ nginx \ curl \ jq \ wget \ supervisor \ apache2-utils \ && rm -rf /var/lib/apt/lists/* + +# Enable pnpm via corepack for runtime +RUN corepack enable🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 36 - 47, The final Dockerfile stage installs system packages but doesn't enable Corepack, so pnpm isn't available for supervisord to run `pnpm run start:server`; after the apt-get install line in the final image (the RUN that installs nginx, curl, jq, wget, supervisor, apache2-utils), add a step to run `corepack enable` (or install/enable corepack before invoking pnpm) so that pnpm is present when supervisord starts and `supervisord.conf` can successfully execute `pnpm run start:server`.
🧹 Nitpick comments (1)
Dockerfile (1)
75-75: Update glob pattern to explicit filename.Since
package-lock.jsonis removed in this PR,package*.jsonnow only matchespackage.json. Consider using the explicit filename for clarity.✨ Suggested change
-COPY package*.json ./ +COPY package.json ./🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 75, The Dockerfile currently uses a glob in the COPY step ("COPY package*.json ./") which now only matches package.json after package-lock.json was removed; update that COPY instruction to explicitly reference package.json (replace the "package*.json" pattern with "package.json") so the intent is clear and future changes don't rely on glob behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 23: The "gen-maps" npm script currently runs "cd map-generator && go run
. && pnpm run format" which causes pnpm to run the root formatter; update the
"gen-maps" script (the package.json "gen-maps" entry) to run the Go formatter
directly by replacing the trailing "pnpm run format" with "go fmt ." so it
becomes "cd map-generator && go run . && go fmt ." to format only the Go files
in map-generator.
In `@supervisord.conf`:
- Line 17: The supervisord command uses "pnpm run start:server" but the final
Docker stage never enables Corepack, so pnpm won't be available; modify the
final image setup (the stage that inherits from node:24-slim / "base") to run
"corepack enable" (or "corepack enable && corepack prepare pnpm@latest
--activate") before supervisord is started so the pnpm CLI is provided at
runtime, ensuring the "command=pnpm run start:server" entry in supervisord
works.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 36-47: The final Dockerfile stage installs system packages but
doesn't enable Corepack, so pnpm isn't available for supervisord to run `pnpm
run start:server`; after the apt-get install line in the final image (the RUN
that installs nginx, curl, jq, wget, supervisor, apache2-utils), add a step to
run `corepack enable` (or install/enable corepack before invoking pnpm) so that
pnpm is present when supervisord starts and `supervisord.conf` can successfully
execute `pnpm run start:server`.
---
Nitpick comments:
In `@Dockerfile`:
- Line 75: The Dockerfile currently uses a glob in the COPY step ("COPY
package*.json ./") which now only matches package.json after package-lock.json
was removed; update that COPY instruction to explicitly reference package.json
(replace the "package*.json" pattern with "package.json") so the intent is clear
and future changes don't rely on glob behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96c188de-26aa-4a8a-a858-1eb41ef2d30b
📒 Files selected for processing (5)
.github/workflows/ci.ymlDockerfileREADME.mdpackage.jsonsupervisord.conf
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
| "prepare": "husky", | ||
| "gen-maps": "cd map-generator && go run . && npm run format", | ||
| "inst": "npm ci --ignore-scripts" | ||
| "gen-maps": "cd map-generator && go run . && pnpm run format", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if map-generator directory exists and has package.json
echo "=== Checking map-generator directory structure ==="
if [ -d "map-generator" ]; then
echo "map-generator directory exists"
if [ -f "map-generator/package.json" ]; then
echo "✓ map-generator has its own package.json"
echo ""
echo "=== Checking for 'format' script in map-generator/package.json ==="
grep -A 50 '"scripts"' map-generator/package.json | head -30
else
echo "✗ No package.json found in map-generator"
fi
else
echo "map-generator directory does not exist"
fiRepository: openfrontio/OpenFrontIO
Length of output: 189
🏁 Script executed:
#!/bin/bash
echo "=== Checking map-generator directory contents ==="
ls -la map-generator/ | head -20
echo ""
echo "=== Checking root package.json format script ==="
grep -A 3 '"format"' package.json
echo ""
echo "=== Checking if there are files to format in map-generator ==="
find map-generator -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" -o -name "*.go" \) 2>/dev/null | head -20Repository: openfrontio/OpenFrontIO
Length of output: 1100
Use go fmt . directly instead of pnpm run format in the gen-maps script.
The pnpm run format command after cd map-generator will not find a package.json in that directory. pnpm walks up to the root and runs the root's format script (prettier --ignore-unknown --write .), which formats the entire repository instead of just the map-generator directory. This is likely unintended.
Since map-generator is a Go-only directory, use go fmt . to format the generated Go code:
"gen-maps": "cd map-generator && go run . && go fmt .",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 23, The "gen-maps" npm script currently runs "cd
map-generator && go run . && pnpm run format" which causes pnpm to run the root
formatter; update the "gen-maps" script (the package.json "gen-maps" entry) to
run the Go formatter directly by replacing the trailing "pnpm run format" with
"go fmt ." so it becomes "cd map-generator && go run . && go fmt ." to format
only the Go files in map-generator.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Dockerfile (2)
30-34: Redundant ignore-scripts setting.Line 30 sets
PNPM_IGNORE_SCRIPTS=1as an environment variable, but line 34 also passes--ignore-scriptsas a CLI flag. The CLI flag alone is enough here since you run only one pnpm command in this stage.🧹 Optional cleanup
# Production dependencies stage - separate from build FROM base AS prod-deps ENV HUSKY=0 -ENV PNPM_IGNORE_SCRIPTS=1 COPY package.json pnpm-lock.yaml ./ RUN --mount=type=cache,target=/root/.local/share/pnpm/store \ corepack enable \ && pnpm install --frozen-lockfile --ignore-scripts --prod🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 30 - 34, Remove the redundant environment variable and rely on the CLI flag: delete the ENV PNPM_IGNORE_SCRIPTS=1 line so the pnpm install invocation (the command containing "pnpm install --frozen-lockfile --ignore-scripts --prod") remains the single source of truth for disabling lifecycle scripts; this keeps the Dockerfile concise and avoids duplicate configuration.
75-75: Update glob pattern to match the pnpm migration.The
package*.jsonglob is an npm-era pattern that would copypackage-lock.jsonif present. Since you are removing the npm lockfile in this PR, consider being explicit:🧹 Clearer copy for pnpm setup
# Copy production node_modules from prod-deps stage (cached separately from build) COPY --from=prod-deps /usr/src/app/node_modules ./node_modules -COPY package*.json ./ +COPY package.json ./If pnpm needs the lockfile at runtime (unlikely for just running the server), also copy
pnpm-lock.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 75, Replace the npm-era glob COPY package*.json ./ with explicit copies so the Docker build only pulls files you intend: update the COPY invocation that currently reads "COPY package*.json ./" to explicitly copy package.json and, if you want to include the pnpm lock for reproducible installs, pnpm-lock.yaml (e.g., COPY package.json pnpm-lock.yaml ./); ensure the Dockerfile still works if pnpm-lock.yaml is absent by either adding it conditionally to your build context or only listing package.json if you don't need the lockfile at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Dockerfile`:
- Around line 30-34: Remove the redundant environment variable and rely on the
CLI flag: delete the ENV PNPM_IGNORE_SCRIPTS=1 line so the pnpm install
invocation (the command containing "pnpm install --frozen-lockfile
--ignore-scripts --prod") remains the single source of truth for disabling
lifecycle scripts; this keeps the Dockerfile concise and avoids duplicate
configuration.
- Line 75: Replace the npm-era glob COPY package*.json ./ with explicit copies
so the Docker build only pulls files you intend: update the COPY invocation that
currently reads "COPY package*.json ./" to explicitly copy package.json and, if
you want to include the pnpm lock for reproducible installs, pnpm-lock.yaml
(e.g., COPY package.json pnpm-lock.yaml ./); ensure the Dockerfile still works
if pnpm-lock.yaml is absent by either adding it conditionally to your build
context or only listing package.json if you don't need the lockfile at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb4f16f7-d3a5-4290-93c4-93810e034a5e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
.github/workflows/ci.ymlDockerfile
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
…date package file copies
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Dockerfile (1)
76-77: Pre-activate pnpm during image build to remove runtime network dependency.Line 77 enables Corepack shims, but the first
pnpmexecution will still need to download the binary from the network. This fails in restricted runtime networks. Pre-activate the exact pnpm version (pnpm@10.30.3from package.json) during the image build.Proposed solution
-# Enable Corepack so pnpm is available for supervisord (command=pnpm run start:server) -RUN corepack enable +# Enable and pre-activate pnpm so runtime does not depend on network +RUN corepack enable \ + && corepack prepare pnpm@10.30.3 --activate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 76 - 77, The Dockerfile currently runs "corepack enable" which leaves the first pnpm invocation to fetch the binary at runtime; replace or augment that step to pre-activate the exact pnpm version from package.json (pnpm@10.30.3) during image build by invoking Corepack prepare for that version and activating it (e.g., use corepack prepare pnpm@10.30.3 --activate after or instead of corepack enable). Update the RUN that contains "corepack enable" so the build image has the pnpm shim and binary preinstalled and no longer needs network access at container runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Dockerfile`:
- Around line 76-77: The Dockerfile currently runs "corepack enable" which
leaves the first pnpm invocation to fetch the binary at runtime; replace or
augment that step to pre-activate the exact pnpm version from package.json
(pnpm@10.30.3) during image build by invoking Corepack prepare for that version
and activating it (e.g., use corepack prepare pnpm@10.30.3 --activate after or
instead of corepack enable). Update the RUN that contains "corepack enable" so
the build image has the pnpm shim and binary preinstalled and no longer needs
network access at container runtime.
- Merge openfrontio/main into chore/migrate-npm-to-pnpm - Resolve conflict: keep package-lock.json removed (using pnpm)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Migrate package management from npm to pnpm (v10)
pnpm-lock.yaml, removepackage-lock.jsonpnpm run inst)node_modules, theninst): npm ~21s vs pnpm ~9sWhy
refimport changed:lit-html→litThis was seen:
The project depends on the unified
litpackage (Lit 3). Imports should come fromlit/..., not from the legacylit-htmlpackage.lit-htmlwas the old, split package (templating only).litis the main package for Lit 2+/3 and re-exportshtml,ref, directives, etc. from a single place.Using
lit/directives/ref.jsis the correct and supported import for the currentlitdependency;lit-html/directives/ref.jsis the deprecated path and can cause inconsistencies or break in future Lit versions.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
.wozniakpl