internal: Migrate Scala.js dep setup from npm to pnpm#4176
Conversation
Why: pnpm uses a content-addressable store with hard-linked node_modules, deduplicating dependencies across worktrees and CI runs. The change keeps the same `jsdom@^27` dependency, switches `setup-scalajs.sh` to `pnpm install`, and adds `pnpm-lock.yaml` so CI installs are deterministic and the pnpm store can be cached via `actions/setup-node`. - scripts/setup-scalajs.sh: replace `npm install jsdom@27.X` with `pnpm install`, with a guard that drops a non-pnpm `node_modules` to keep the script non-interactive on existing checkouts. - .github/workflows/test.yml: add `pnpm/action-setup@v4` and enable `cache: 'pnpm'` in the four jobs that run setup-scalajs.sh. - airframe-rx-widget/README.md: update dev instructions to pnpm; use `pnpm dlx browser-sync` instead of a global install. - pnpm-lock.yaml: committed for reproducible installs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
doc
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the repository's dependency management from npm to pnpm to improve storage efficiency and CI performance. The changes include updating the Scala.js setup script, adding a pnpm-lock.yaml file, and revising the developer documentation. Feedback highlights that the required updates to the GitHub Actions workflow files are missing from the PR. Additionally, the migration plan document contains an invalid version for actions/setup-node and an inconsistency regarding the recommended command for running browser-sync.
| storage-savings story: with it, CI installs are deterministic and the pnpm content-addressable | ||
| store can be cached across runs (`actions/setup-node` `cache: 'pnpm'`). | ||
|
|
||
| ### 3. `.github/workflows/test.yml` |
There was a problem hiding this comment.
The changes to .github/workflows/test.yml described in this section and the PR summary are missing from the actual file changes in this pull request. Without these updates to include pnpm/action-setup and configure the pnpm cache in actions/setup-node, the CI jobs will likely fail or not benefit from the intended caching improvements.
| - uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: 9 | ||
| - uses: actions/setup-node@v6 |
There was a problem hiding this comment.
actions/setup-node@v6 is not a valid version; the current major version is v4. Using a non-existent version will cause the GitHub Action to fail. This also applies to the mention on line 51.
| - uses: actions/setup-node@v6 | |
| - uses: actions/setup-node@v4 |
| `browser-sync` global install line stays as `npm install -g browser-sync` only if pnpm has | ||
| no equivalent — pnpm does have `pnpm add -g browser-sync`, so use that. |
There was a problem hiding this comment.
There is an inconsistency between this plan and the implementation in airframe-rx-widget/README.md. The plan suggests using pnpm add -g browser-sync, but the README correctly uses pnpm dlx browser-sync. pnpm dlx is generally preferred for running tools without global installation. This document should be updated to reflect the actual implementation.
Why: After moving Scala.js setup to pnpm with a committed lockfile, a PR that only updates package.json or pnpm-lock.yaml would have left needs.changes.outputs.tests == false, skipping the four jobs that actually exercise setup-scalajs.sh. Add the pnpm-related paths to the filter so dependency-only changes still run the Scala.js tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
npm install jsdom@27.Xwithpnpm installinscripts/setup-scalajs.sh. pnpm's content-addressable store hard-links files intonode_modules, deduplicating disk usage across worktrees and CI runs.pnpm-lock.yamlso installs are deterministic; enablecache: 'pnpm'on the fouractions/setup-nodejobs that run the setup script.testspath filter intest.ymlso dependency-only changes (package.json,pnpm-lock.yaml, orsetup-scalajs.sh) still trigger the Scala.js / integration jobs that exercise them.airframe-rx-widget/README.mdto usepnpm installandpnpm dlx browser-sync(avoids requiring a global pnpm setup).node_modulesso existing developer environments upgrade non-interactively.jsdomstays at^27.0.0frompackage.json, so behavior is equivalent to the oldnpm install jsdom@27.Xline.Test plan
./scripts/setup-scalajs.shsucceeds locally on a clean checkout../scripts/setup-scalajs.shsucceeds when re-run (lockfile path)../scripts/setup-scalajs.shsucceeds when an npm-creatednode_modulesexists.test_integration,test_js,test_js_2_13,test_js_3jobs pass on CI.🤖 Generated with Claude Code