chore: migrate from yarn to pnpm #913
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReplaces Yarn with PNPM across CI workflows, containerfiles, scripts, Gitpod, docs, and tooling; switches lockfile tracking from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/develop.md (1)
8-25: Align command formatting for consistencyThe three pnpm commands are correct, but
pnpm desk:build(Line 25) has two leading spaces while the others don’t. For consistency and copy‑paste friendliness, consider removing the extra spaces so all three commands are formatted the same way..gitpod.yml (1)
7-9: Consider explicitly ensuring pnpm is available in GitpodThe
pnpm install,pnpm run build, andpnpm run watchcommands look fine, but this assumes pnpm is already available in the Gitpod workspace image. To make this more robust and future‑proof, consider adding an explicit pnpm setup step (e.g., viacorepack enable pnpmornpm install -g pnpm) or documenting that the chosen Gitpod image includes pnpm.oci/Containerfile.multistage (1)
22-25: Align container build with lockfile‑based installsUsing global pnpm and
pnpm i && pnpm buildworks, but you lose lockfile enforcement here whileoci/Containerfile.builderusespnpm i --frozen-lockfile. For more reproducible builds, consider updating this to:RUN npm install -g pnpm RUN pnpm i --frozen-lockfile \ && pnpm buildand, if possible, pinning the pnpm version instead of installing the latest globally.
scripts/build.cjs (1)
47-48: Redundant working‑directory configuration in pnpm call
pnpm add hasha@^6.0.0 --dir .combined with{ cwd: './dist' }effectively sets the project directory todisttwice. It’s harmless but a bit confusing.Consider simplifying to one source of truth, e.g.:
cproc.exec('pnpm add hasha@^6.0.0', { cwd: './dist' }, (error, stdout, stderr) => { // ... });to rely solely on
cwd.oci/Containerfile.builder (1)
18-25: Good lockfile usage; consider pinning pnpm and validating Node 22 upgradeSwitching to
ubi10/nodejs-22and runningpnpm i --frozen-lockfileis a solid move for deterministic installs. Two follow‑ups to consider:
- Pin the pnpm version instead of
npm install -g pnpm(e.g.,npm install -g pnpm@<version>) to avoid surprise breakages.- Double‑check that the extension build and runtime are happy with Node.js 22, since this is a non‑trivial jump from the previous Node 20 base.
Both are precautionary rather than blockers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.extfiles(1 hunks).github/workflows/e2e-main.yml(4 hunks).github/workflows/pr-check.yaml(7 hunks).gitignore(0 hunks).gitpod.yml(1 hunks)docs/develop.md(2 hunks)oci/Containerfile.builder(1 hunks)oci/Containerfile.multistage(1 hunks)scripts/build.cjs(1 hunks)scripts/run.mts(2 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/pr-check.yaml
82-82: property "cachenodemodules" is not defined in object type {pnpm-cache: {conclusion: string; outcome: string; outputs: {cache-hit: string}}; pnpm-cache-dir-path: {conclusion: string; outcome: string; outputs: {string => string}}}
(expression)
🪛 RuboCop (1.81.7)
oci/Containerfile.builder
[fatal] 23-23: unexpected token tIDENTIFIER
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
🔇 Additional comments (5)
.extfiles (1)
5-6: Lockfile exclusion is consistent and correctExcluding
dist/pnpm-lock.yamlwhile includingdist/**keeps the extension payload clean and mirrors typical handling of lockfiles; no issues here..github/workflows/e2e-main.yml (1)
85-89: pnpm migration in e2e workflow looks consistent across OSesThe replacements of
yarnwithpnpmfor install (pnpm install --check-files), build (pnpm build/pnpm test:e2e:build), and test (pnpm test:e2e) are coherent and consistent for Ubuntu, macOS, and Windows, and align with the earlierpnpm/action-setupstep. No additional issues from this change.Also applies to: 130-171, 204-209
scripts/run.mts (1)
72-77: FixprepareDevto callpnpm installexplicitly (current code will fail)
prepareDevcurrently does:await exec('pnpm', undefined, { cwd: desktopPath }); await exec('pnpm', [], { cwd: path.join(__dirname, '..') });Unlike Yarn (where bare
yarnrunsyarn install), runningpnpmwithout a subcommand shows help and exits with a non-zero code. This will causedesk:prepareto fail.Explicitly call
installin both locations:async function prepareDev() { await checkAndCloneDesktopRepo(); - await exec('pnpm', undefined, { cwd: desktopPath }); - await exec('pnpm', [], { cwd: path.join(__dirname, '..') }); + await exec('pnpm', ['install'], { cwd: desktopPath }); + await exec('pnpm', ['install'], { cwd: path.join(__dirname, '..') }); }The other script usages (
pnpm compile:current,pnpm build,pnpm watch) are consistent with prior Yarn usage..github/workflows/pr-check.yaml (2)
36-120: Yarn-to-PNPM migration is well-executed overall.The systematic replacement across all three primary jobs (Windows, macOS, lint-format-unit) is consistent: cache setup uses
pnpm cache dir, lockfile keys referencepnpm.lock, and all commands are correct (e.g.,pnpm --frozen-lockfile,pnpm build,pnpm lint:check,pnpm format:check,pnpm test). Once the cache-hit condition is fixed, this workflow should be solid.
158-287: E2E test job correctly integrated PNPM tooling and build commands.The multi-platform e2e-tests job properly installs pnpm via the dedicated action (line 159), and all build and test steps consistently use pnpm commands. Cross-platform builds for Ubuntu, macOS, and Windows all invoke
pnpm install --check-files,pnpm build, andpnpm test:e2ecorrectly.
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
What is the rationale for this change ? Are we sure this is the right timing ?
|
Probably after the 2.1 release it would be better time, but still I see this as "deprecated" thing |
gastoner
force-pushed
the
migrate_to_pnpm
branch
3 times, most recently
from
606867f to
13a150f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/pr-check.yaml (1)
73-83: Align macOS cache‑hit condition with updated step id.The
ifexpression still referencessteps.cacheNodeModules.outputs.cache-hit, but the cache step id is nowpnpm-cache, so the condition never inspects the real cache result (and actionlint flags this).You can either rename the step back to
cacheNodeModulesor update the condition. To keep the current id, apply:- uses: actions/cache@v4 - id: pnpm-cache + id: pnpm-cache with: path: ${{ steps.pnpm-cache-dir-path.outputs.dir }} key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm-lock.yaml') }} restore-keys: | ${{ runner.os }}-pnpm- - name: Execute pnpm - if: ${{ steps.cacheNodeModules.outputs.cache-hit != 'true' }} + if: ${{ steps.pnpm-cache.outputs.cache-hit != 'true' }} run: pnpm --frozen-lockfile --network-timeout 180000
🧹 Nitpick comments (2)
oci/Containerfile.builder (1)
22-26: pnpm builder flow looks good; consider pinning pnpm version for reproducibility.Copying
pnpm-lock.yamland usingpnpm i --frozen-lockfileis the right pattern. To avoid future breakage from pnpm major upgrades in this base image, consider pinning the pnpm version, for example:-RUN npm install -g pnpm +RUN npm install -g pnpm@<desired-major>oci/Containerfile.multistage (1)
22-25: Multi‑stage builder correctly uses pnpm; pin pnpm version for consistency with builder.The
npm install -g pnpm+pnpm i && pnpm buildflow matches the dedicated builder image. For deterministic builds, it would be safer to pin pnpm here as well to the same version used inoci/Containerfile.builder.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.extfiles(1 hunks).github/workflows/e2e-main.yml(4 hunks).github/workflows/pr-check.yaml(7 hunks).gitignore(0 hunks).gitpod.yml(1 hunks)docs/develop.md(2 hunks)oci/Containerfile.builder(1 hunks)oci/Containerfile.multistage(1 hunks)scripts/build.cjs(1 hunks)scripts/run.mts(2 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/e2e-main.yml
- docs/develop.md
- scripts/run.mts
- .extfiles
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/pr-check.yaml
82-82: property "cachenodemodules" is not defined in object type {pnpm-cache: {conclusion: string; outcome: string; outputs: {cache-hit: string}}; pnpm-cache-dir-path: {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). (2)
- GitHub Check: e2e tests for macos-14
- GitHub Check: e2e tests for windows-2022
🔇 Additional comments (4)
.gitpod.yml (1)
8-9: Gitpod tasks correctly migrated to pnpm.Switching init and watch commands to pnpm is consistent with the repo-wide migration; nothing else to adjust here.
scripts/build.cjs (1)
48-48: CLI switch from Yarn to pnpm is correct here.Using
pnpm add hasha@^6.0.0 --dir .withcwd: './dist'preserves the previous behavior of installinghashainto thedistfolder; no further changes needed..github/workflows/pr-check.yaml (2)
205-225: E2E build and test steps are cleanly migrated to pnpm.The
pnpm install --check-files,pnpm build, andpnpm test:e2eusages in the Ubuntu, macOS, and Windows E2E flows are consistent with the rest of the pipeline and correctly replace the former Yarn commands.Also applies to: 232-241, 247-257, 283-288
36-54: Ensure pnpm is installed in non‑E2E jobs before calling it.In the
windows,darwin, andlint-format-unitjobs, you callpnpm cache dir,pnpm --frozen-lockfile,pnpm build,pnpm lint:check,pnpm format:check, andpnpm testwithout any prior pnpm setup step. GitHub-hosted runners do not pre-install pnpm, so these steps will fail unless explicitly configured.For robustness and consistency with the
e2e-testsjob, add a pnpm setup step afteractions/setup-nodein each job:- uses: actions/setup-node@v6 with: node-version: 22 + + - name: Setup pnpm + uses: pnpm/action-setup@v4 + with: + run_install: falseKeep your explicit
pnpm --frozen-lockfilestep as-is.Also applies to: 69–87, 98–120
.github/workflows/pr-check.yaml
Outdated
| key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm.lock') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-yarn- | ||
| ${{ runner.os }}-pnpm- | ||
There was a problem hiding this comment.
Fix cache key to use the correct pnpm lockfile name.
The cache keys currently hash **/pnpm.lock, but pnpm’s default lockfile is pnpm-lock.yaml. As written, hashFiles('**/pnpm.lock') will not find your lockfile and will reduce cache effectiveness.
Update the keys in all three jobs to:
- key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm.lock') }}
+ key: ${{ runner.os }}-pnpm-${{ hashFiles('**/pnpm-lock.yaml') }}Also applies to: 77-79, 106-108
🤖 Prompt for AI Agents
.github/workflows/pr-check.yaml around lines 44-47 (and also update the same
pattern at 77-79 and 106-108): the cache key is hashing the wrong pnpm lockfile
name (`**/pnpm.lock`) so the lockfile won't be found; update all three jobs to
use pnpm's default lockfile pattern `pnpm-lock.yaml` in the hashFiles call
(e.g., change `hashFiles('**/pnpm.lock')` to `hashFiles('**/pnpm-lock.yaml')`)
and adjust any related restore-keys if they include the lockfile name.
14d2801 to
d8bd068
Compare
| - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | ||
|
|
||
| - uses: actions/setup-node@v6 | ||
| - uses: pnpm/action-setup@a7487c7e89a18df4991f7f222e4898a00d66ddda # v4.1.0 |
There was a problem hiding this comment.
Shouldn't be node installed first?
gastoner
force-pushed
the
migrate_to_pnpm
branch
4 times, most recently
from
f3767ad to
c6f5779
Compare
gastoner
force-pushed
the
migrate_to_pnpm
branch
3 times, most recently
from
270f6aa to
c08a567
Compare
Signed-off-by: Evzen Gasta <[email protected]>
gastoner
force-pushed
the
migrate_to_pnpm
branch
from
c08a567 to
5718ef2
Compare
3 participants
Migrates from yarn to pnpm that is used in Podman Desktop
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.