SF-3821 Switch to pnpm#3966
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3966 +/- ##
==========================================
- Coverage 81.03% 81.02% -0.02%
==========================================
Files 645 645
Lines 41463 41469 +6
Branches 6750 6751 +1
==========================================
Hits 33600 33600
- Misses 6755 6760 +5
- Partials 1108 1109 +1 ☔ View full report in Codecov by Harness. |
0e66059 to
58f9dea
Compare
61225c8 to
c54acaa
Compare
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
faaff4f to
09a11ea
Compare
09a11ea to
a2233b6
Compare
a2233b6 to
52a0dad
Compare
52a0dad to
586ffb3
Compare
|
This is reviewable in Devin Review. |
There was a problem hiding this comment.
Pull request overview
This PR migrates the repo’s JavaScript/TypeScript tooling (RealtimeServer, ClientApp, and db_tools) from npm to pnpm, updating local/devcontainer workflows, CI pipelines, and .NET build integration so the realtime server assets/deps are built/copied correctly under pnpm’s node_modules layout.
Changes:
- Replace npm usage with pnpm across build/test scripts, Dockerfiles, devcontainer setup, and GitHub Actions workflows.
- Update .NET/MSBuild targets to build/copy the RealtimeServer and re-install dependencies in output folders to avoid pnpm symlink copy issues.
- Add pnpm lock/workspace artifacts and adjust test harness/proxies to keep frontend tests stable.
Reviewed changes
Copilot reviewed 53 out of 64 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| test/SIL.XForge.Tests/SIL.XForge.Tests.csproj | Switch RealtimeServer build/install steps to pnpm and adjust copied content. |
| test/SIL.XForge.Scripture.Tests/SIL.XForge.Scripture.Tests.csproj | Add pnpm-based RealtimeServer build/copy steps for Scripture tests. |
| src/SIL.XForge/Realtime/RealtimeServiceCollectionExtensions.cs | Point NodeJS ProjectPath at copied RealtimeServer/ subfolder. |
| src/SIL.XForge/Realtime/MemoryRealtimeService.cs | Align in-memory NodeJS service ProjectPath with copied RealtimeServer/ folder. |
| src/SIL.XForge.Scripture/SIL.XForge.Scripture.csproj | Use pnpm for SPA/RealtimeServer restore/build and add hoisted installs for publish/build outputs. |
| src/SIL.XForge.Scripture/Dockerfile | Install pnpm in image and use pnpm for dependency installation. |
| src/SIL.XForge.Scripture/ClientApp/src/material-styles.scss | Minor style formatting tweak. |
| src/SIL.XForge.Scripture/ClientApp/src/karma.conf.js | Add additional proxy stubs for test asset paths. |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-history/editor-history.component.spec.ts | Provide AuthService mock for editor history tests. |
| src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-draft/editor-draft.component.spec.ts | Provide AuthService mock for editor draft tests. |
| src/SIL.XForge.Scripture/ClientApp/pnpm-workspace.yaml | Introduce pnpm workspace/config file (needs validation vs pnpm schema). |
| src/SIL.XForge.Scripture/ClientApp/patches/http-proxy+1.18.1.patch | Remove patch-package style patch file. |
| src/SIL.XForge.Scripture/ClientApp/patches/http-proxy@1.18.1.patch | Add pnpm-style patch file for http-proxy. |
| src/SIL.XForge.Scripture/ClientApp/package.json | Update scripts/deps for pnpm and adjust dependency versions. |
| src/SIL.XForge.Scripture/ClientApp/.prettierignore | Ignore additional generated directories. |
| src/SIL.XForge.Scripture/ClientApp/.npmrc | Remove npm-specific config file. |
| src/RealtimeServer/start.sh | Add a start wrapper that runs migrator then launches the server. |
| src/RealtimeServer/pnpm-workspace.yaml | Introduce pnpm workspace/config file (needs validation vs pnpm schema). |
| src/RealtimeServer/package.json | Update scripts to invoke via pnpm and adjust devDependencies. |
| src/RealtimeServer/Dockerfile | Install pnpm and use it for CI/build steps in container builds. |
| src/RealtimeServer/.npmrc | Remove npm-specific config file. |
| scripts/db_tools/validate-data.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/trigger-resources-need-sync.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/show-projects.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/show-jwt.ts | Add JWT inspection helper. |
| scripts/db_tools/report-user-edits.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/report-user-comments.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/README.md | Update db_tools instructions to pnpm. |
| scripts/db_tools/pnpm-workspace.yaml | Introduce pnpm workspace/config file (needs validation vs pnpm schema). |
| scripts/db_tools/pnpm-lock.yaml | Add pnpm lockfile for db_tools dependencies. |
| scripts/db_tools/ping-rts.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/parse-version.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/package.json | Add typescript devDependency consistent with pnpm lock. |
| scripts/db_tools/manipulate-sharedb.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/manage-db.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/import-project.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/find-problem-cids.ts | Update setup instructions/comments to pnpm. |
| scripts/db_tools/duplicate-doc.js | Add helper to duplicate docs with new ID format. |
| scripts/db_tools/db-token-info.ts | Update setup instructions/comments to pnpm and reuse show-jwt helper. |
| scripts/db_tools/.npmrc | Remove npm-specific config file. |
| deploy/vagrant/sfdev/Vagrantfile | Install pnpm globally in vagrant provisioning. |
| AGENTS.md | Update repo guidance to use pnpm for frontend tests. |
| .vscode/tasks.json | Update VS Code tasks to pnpm equivalents. |
| .vscode/launch.json | Update launch configs to reference pnpm-based tasks/commands. |
| .gitignore | Ignore pnpm store directory. |
| .github/workflows/update-npm-audit.yml | Convert dependency update workflow to pnpm-based auditing. |
| .github/workflows/storybook-tests.yml | Run Storybook build/tests using pnpm in CI. |
| .github/workflows/storybook-screenshots.yml | Add pnpm-based screenshot job paths and base/branch logic. |
| .github/workflows/release.yml | Add pnpm version input and pnpm caching/setup for release builds. |
| .github/workflows/release-qa.yml | Add pnpm version to QA release matrix. |
| .github/workflows/release-live.yml | Add pnpm version to Live release matrix. |
| .github/workflows/e2e-tests.yml | Update e2e prep steps to pnpm and add pnpm to matrices. |
| .github/workflows/codeql.yml | Install dependencies with pnpm prior to CodeQL init. |
| .github/workflows/ci-production-build.yml | Switch production build workflow installs/builds to pnpm. |
| .github/workflows/ci-lint-prettier.yml | Switch lint/prettier workflow installs/runs to pnpm. |
| .github/workflows/build-and-test.yml | Switch main build/test workflow installs/builds/tests to pnpm. |
| .devcontainer/README.md | Update devcontainer test instructions to pnpm. |
| .devcontainer/post-create.sh | Use pnpm for post-create dependency installs. |
| .devcontainer/Dockerfile | Install pnpm into devcontainer image. |
Files not reviewed (1)
- scripts/db_tools/pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Target Name="InstallRealtimeServerDependencies" AfterTargets="Build"> | ||
| <!-- Fix the node_modules as MSBuild will not correctly copy the symlinks used by pnpm --> | ||
| <Exec WorkingDirectory="$(OutDir)RealtimeServer\" Command="pnpm install" Condition="!Exists('$(OutDir)RealtimeServer\node_modules') Or '$(NpmInstall)' == 'true'" /> | ||
| </Target> |
There was a problem hiding this comment.
This is not true as the BuildRealtimeServer target executes on Release and Debug builds.
There was a problem hiding this comment.
I see that the BuildRealtimeServer in SIL.XForge.Scripture.csproj is not marked to only run when Debug. Is it significant that the BuildRealtimeServer target in this file is marked to only run when Debug, yet InstallRealtimeServerDependencies is not similarly limited?
There was a problem hiding this comment.
Setting as Blocking to keep the thread afloat above Resolved etc threads.
There was a problem hiding this comment.
Done. I was looking at the wrong csproj :-/
|
I am in-progress reviewing this PR. |
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 57 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on pmachapman).
.github/workflows/storybook-screenshots.yml line 35 at r6 (raw file):
# Resolve the base ref in priority order: PR base SHA → merge-group base SHA → manual # input → default branch. ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || inputs.base_ref || 'master' }}
Why is all this needed instead of just the pull_request base sha?
| <Target Name="InstallRealtimeServerDependencies" AfterTargets="Build"> | ||
| <!-- Fix the node_modules as MSBuild will not correctly copy the symlinks used by pnpm --> | ||
| <Exec WorkingDirectory="$(OutDir)RealtimeServer\" Command="pnpm install" Condition="!Exists('$(OutDir)RealtimeServer\node_modules') Or '$(NpmInstall)' == 'true'" /> | ||
| </Target> |
There was a problem hiding this comment.
Setting as Blocking to keep the thread afloat above Resolved etc threads.
4c0bf3c to
973d9d9
Compare
fd7e6c1 to
e66b9a9
Compare
e66b9a9 to
d3548a2
Compare
|
📸 Screenshot diff deployed! (2 changes) View the visual diff at: https://pr-3966--sf-screenshot-diffs.netlify.app |
90c7781 to
0f20eaa
Compare
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman made 8 comments.
Reviewable status: 47 of 66 files reviewed, 8 unresolved discussions (waiting on marksvc).
.github/workflows/storybook-screenshots.yml line 293 at r4 (raw file):
Previously, marksvc wrote…
Two code review tools are concerned that this will result in nothing happening, since screenshots-base and screenshots-base-npm are exclusive of each other's operation.
Let's confirm that this is working as intended.
Done. I have fixed the screenshots.
.github/workflows/storybook-screenshots.yml line 35 at r6 (raw file):
Previously, marksvc wrote…
Why is all this needed instead of just the pull_request base sha?
I replicated this from the screenshots-base-npm: job. It looks like it was auto-created by Copilot in the original PR (#3825)
src/RealtimeServer/Dockerfile line 28 at r4 (raw file):
Previously, marksvc wrote…
Will this FROM section will need an
ARG PNPM_VERSIONfor this variable to have a value here?
Done. Yes, it should.
src/SIL.XForge.Scripture/Dockerfile line 16 at r4 (raw file):
Previously, marksvc wrote…
I find
apt-get update --fix-missingto be interesting here. What is your motivation to add this argument here?I see from the man page that --fix-missing will
Ignore missing packages; if packages cannot be retrieved or fail the integrity check after retrieval (corrupted package files), hold back those packages and handle the result
Is there a reason we will not be getting a clean
apt-get updaterun, or that we would want to ignore missing or corrupted results?
I was having issues with the mirrors in NZ being out of date. That seems to be resolved, so I have changed this to a normal apt-get update. If I see Docker builds failing in future, I will take a look at this again.
src/SIL.XForge.Scripture/Dockerfile line 26 at r4 (raw file):
Previously, marksvc wrote…
Does this FROM section need
ARG PNPM_VERSIONfor this to have a value?
Done. Yes, it does.
src/SIL.XForge.Scripture/Dockerfile line 57 at r4 (raw file):
Previously, marksvc wrote…
(And similarly here?)
Done. Thank you!
test/SIL.XForge.Scripture.Tests/SIL.XForge.Scripture.Tests.csproj line 64 at r4 (raw file):
Previously, marksvc wrote…
I see that in SIL.XForge.Scripture.csproj, the similar stanzas used --node-linker hoisted. Here and in SIL.XForge.Tests.csproj, there is no
--node-linker hoisted. Is that intentional?
Yes, that is intentional. --node-linker hoisted is only required for Publish, not Build.
| <Target Name="InstallRealtimeServerDependencies" AfterTargets="Build"> | ||
| <!-- Fix the node_modules as MSBuild will not correctly copy the symlinks used by pnpm --> | ||
| <Exec WorkingDirectory="$(OutDir)RealtimeServer\" Command="pnpm install" Condition="!Exists('$(OutDir)RealtimeServer\node_modules') Or '$(NpmInstall)' == 'true'" /> | ||
| </Target> |
There was a problem hiding this comment.
Done. I was looking at the wrong csproj :-/
Perform running locally, update to use pnpm:
Linux
Windows
Navigate to your Scripture Forge directory, then:
When you move back to your previous branch, you should run:
Linux
Windows
This change is