Stop shipping build-only deps in the app - #23
Draft
mokagio wants to merge 1 commit into
Draft
Conversation
The renderer is esbuild-bundled at build time, so React, `@wordpress/*`, `@emotion/*`, and `xterm` are already inlined into `src/renderer`. Sitting in `dependencies`, electron-builder also shipped their full `node_modules` trees inside the app — dead weight the runtime never loads. Move them to `devDependencies` so packaging drops them, and remove `@xterm/xterm`, which has no references in `src` (the renderer imports the `xterm` package). Trims the packaged production tree from 1366 MB to 1196 MB (~170 MB, 12%). The bulk that remains is `@php-wasm` (PHP-WASM builds under `@wp-playground/cli`); reducing that needs a call on which PHP versions and execution strategies to support and is left as a follow-up. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mokagio
changed the base branch from
ainfra-2597-fix-macos-code-signing-for-contributor-toolkit
to
trunk
July 10, 2026 11:05
mokagio
force-pushed
the
mokagio/trim-desktop-artifact-deps
branch
from
July 10, 2026 11:05
b6ae55b to
1f8ca57
Compare
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
) ## Why The app's user documentation is the README, and it has outgrown it: install steps, a ten-step walkthrough and the trunk-update rules all compete for the same page, and there is nowhere to put a screenshot. This PR gives the guide a home and a deploy pipeline. It carries no guide content beyond the landing page — that arrives in #232, and the harness that photographs the app in #231. ## What changes - `docs/` becomes a VitePress site with **its own npm package**, so a docs-only CI job never runs the root `postinstall` (electron-builder + esbuild) and the app's dependency tree stays free of a static-site generator. - `.github/workflows/docs.yml` builds and deploys to GitHub Pages on pushes to `trunk` that touch `docs/`. Pull requests get a **build-only** job: a dead link fails before merge, and the job holding the OIDC token never runs on PR code. - The site's `base` is derived from the repository name at run time. A project Pages site is served under `/<repo-name>/`, and this repo has already been renamed once — deriving it means another rename cannot break every asset URL. - `srcExclude` keeps `docs/testing.md` (from #70) out of the published site: it documents how to run the suites, which is contributor material, and this site is for users of the app. ## How to test this **Starting state:** this branch checked out, `npm ci` done. 1. `npm run docs:build` — expect `build complete`. Now add a link to a page that does not exist in `docs/index.md` and run it again: it must **fail** with `dead link(s) found`. Undo. 2. `npm run docs:preview`, open the printed URL — the home page renders with the WordPress Contributor Toolkit hero and the sidebar. Clean URLs work; this is exactly what Pages serves. 3. `npm run docs:dev` — pages are served at their `.html` paths in dev (`/index.html`). With the dev server open, run `npm run docs:build` in another terminal: the dev server must **not** spew reload lines (it ignores its own output directory). 4. `npm run lint` and `npm test` — both green, unchanged from `trunk`. **Must not have happened:** no `deploy` job may run on this pull request — check the Actions tab and confirm only `build site` ran. **Not testable by hand here:** the deploy itself. It needs the workflow on `trunk`, and Pages is already set to "GitHub Actions" as its source. ## Risks Merging this alone deploys a site whose sidebar names the full guide, so **those links 404 until the content PR lands**. Merge the stack in order rather than leaving this on `trunk` by itself. The deploy job holds `pages: write` and `id-token: write`. Its actions are pinned to commit SHAs rather than tags, following the reasoning already written down in `download-stats.yml`. ## Related Stack: this → #231 (screenshot harness) → #232 (guide content). Touches #70 only through `srcExclude`; that PR needs no change. --- <details> <summary><b>Self-review</b> — 5 findings, all fixed</summary> Ran `.github/instructions/code-review.instructions.md` with the judgement pass in a fresh context. Deterministic layer was clean (ESLint and the unit suite on macOS and Windows). **5 [fix here] · 1 [follow-up]. All five fixed in this branch:** | Dimension | Was | Now | |---|---|---| | security 🟡 | `checkout` kept the job's `GITHUB_TOKEN` in `.git/config` while the job runs PR-authored code (`npm ci` with lifecycle scripts, and a VitePress build that evaluates `config.mjs`) | `persist-credentials: false`, matching `lint.yml` | | security 🔵 | `if: github.event_name != 'pull_request'` let a `workflow_dispatch` on any ref publish to the live site, which is not what the comment claimed | `if: github.ref == 'refs/heads/trunk'` | | cross-platform 🔵 | `node-version: 22` hardcoded while `.nvmrc` says 24.18.0 — reintroducing the Node drift of #37/#46 for the one command CONTRIBUTING calls "what CI runs" | `node-version-file: .nvmrc` | | architecture 🔵 | one concurrency group spanning build and deploy, with `cancel-in-progress: true`, so a second push could cancel an in-flight `deploy-pages` | split: builds cancel, deploys do not | | architecture 🔵 | CONTRIBUTING documented `docs:*` without saying the nested package needs its own install, so the commands fail on a clean clone | `npm ci --prefix docs` documented | **Deferred [follow-up]:** `electron-builder` has no `files` filter, so tracked `docs/` sources ship inside `app.asar` — and #232 adds screenshots on top. Real, pre-existing, and overlaps #23; an `!docs{,/**/*}` entry closes it. Not done here because it changes what every release artifact contains, which deserves its own PR and its own testing. **Checked and clean:** all four action pins resolve to the tags their comments claim (verified against the GitHub API); `pull_request_target` is correctly not used and no fork PR can reach `pages: write` / `id-token: write`; the lockfile is 175 packages, all from registry.npmjs.org, with install scripts only on esbuild and fsevents; the `import/no-unresolved` exemption is a genuine false positive and hides nothing; no `src/` code, IPC surface or spawn path is touched. </details>
This was referenced Aug 11, 2026
juanmaguitar
added a commit
that referenced
this pull request
Aug 11, 2026
## Why No branch has produced a signed macOS or Windows artifact since the last version bump. The build dies during packaging: ``` ⨯ ENOENT: no such file or directory, open '…/vendor/npm/_logs/2026-08-10T14_15_09_297Z-debug-0.log' ``` Trunk fails the same way, re-running a job does not help, and reviewers have had no way to test a pull request on a real machine — the only reason that build layer exists. Linux is unaffected, which is the first clue: it is the one job that does not use this cache. Details in #265. ## What changes **Root cause.** The artifact build keeps npm's package cache at `vendor/npm`, *inside the project directory*, and npm writes its own debug logs into it. `package.json` had no `files` filter, so electron-builder's default of "everything in the project directory is application content" swept that cache into the app. A cache directory is not static while a build runs: a log file present when the walk begins and gone by the time it is read takes the whole build down. Two changes, addressing two separate mistakes: - **`package.json` — exclude `vendor` from packaging.** This is the fix. A directory that mutates during the build must not be a packaging input. The `files` array holds only a negation, which electron-builder handles by prepending its own `**/*` default (`fileMatcher.js` → `containsOnlyIgnore()`), so nothing else about what gets packaged changes. The `!x{,/**/*}` form is the one electron-builder uses internally for the same job. - **The CI script — clear npm's logs on both sides of the cache.** After the restore, so no build inherits logs written by another build on another machine; before the save, so none are archived onward. This is cache hygiene, not the cure: `npm ci` writes a fresh log during the build regardless, so only the exclusion above closes the failure. `.gitignore` gains `vendor/npm` alongside the existing `vendor/bundle`, so the cache cannot be committed by anyone who runs the CI script locally. **Why the version bump was the trigger.** The cache key hashes `package-lock.json`. The bump minted a fresh cache entry, and the build that created it archived its own debug log into it — the timestamp in the error is that build's `npm ci`. Every build since has restored the poisoned entry. ## How to test this **Platforms: macOS and Windows.** These are the two jobs that use this cache; Linux never had the problem. The end-to-end test is this PR's own artifact build. Locally, packaging can be driven directly: **Starting state:** a clean checkout with dependencies installed. 1. Stage a stand-in for the CI cache: ``` mkdir -p vendor/npm/_logs && echo x > vendor/npm/_logs/stale-debug-0.log ``` 2. `CSC_IDENTITY_AUTO_DISCOVERY=false npx electron-builder --dir --publish never` 3. `npx asar list "dist/mac-arm64/WordPress Contributor Toolkit.app/Contents/Resources/app.asar" | grep '^/vendor'` **Expected: no output.** On `trunk` the same command lists `/vendor`, `/vendor/npm`, `/vendor/npm/_logs` and the log file itself — that is the bug, visible without needing to reproduce the race. 4. Same command without the `grep`, confirming the app is still whole: `/src/main.js`, `/src/preload.js`, `/src/renderer/index.js`, `/package.json` and the `node_modules` tree are all present. **What must not have happened:** the app must not have got *smaller* in any way other than losing `vendor`. A wrong `files` pattern would silently drop `src` or `node_modules` and the packaging step would still report success — the app would only fail when a contributor launches it. Step 4 is what catches that. Also check the CI log still shows the npm cache being restored and saved: the cleanup must not have removed `_cacache`, or every build re-downloads every package. ## Risks and limitations - **The crash itself was not reproduced locally.** It needs the log file to vanish mid-walk, which is a race between processes on the CI machine and could not be staged. What is verified is the mechanism underneath it: the cache directory *is* packaged, and it *does* contain those logs. The green build on this PR is the confirmation. - **`vendor` is excluded; other non-app directories are not.** `docs`, `test`, `scripts` and `fastlane` still get packaged, but they are ~217 entries against 28085 for `node_modules`, so there is nothing to win there. The real artifact bulk is the dependency tree — see #23, which measured it and is a separate concern. - **No unit test.** Neither an electron-builder config key nor a CI shell script is reachable from `node --test`; a test asserting the string `"!vendor{,/**/*}"` is present would restate the diff rather than test it. Verified by actually packaging instead, twice, and the assertion is written into the testing steps above so it can be re-run. - The published artifacts up to now have been carrying the build machine's package cache. Whether that is a meaningful share of their size will show when this build's artifact size is compared against the last one. ## Related Fixes #265. Adjacent to #23 (packaged app carries more than the app), which attacks artifact size through the dependency tree and is stale and conflicting — it needs its own decision. --- <details> <summary>Design decisions and alternatives considered</summary> **Moving npm's cache out of the project directory** would have fixed the crash at its root, and was the first instinct. Rejected because the cache path is load-bearing for the CI cache save/restore, which archives that directory by relative path — relocating it means reworking the caching, on a pipeline that is currently red. Excluding it from packaging is the smaller change and the correct invariant regardless of where the cache lives. **Setting npm's `logs-dir` outside the project** is the tidier version of the log cleanup and would make the deletions unnecessary. Not done here because it needs a path that is valid on both the macOS and Windows agents, which is a second decision to get wrong while the pipeline is broken. Worth revisiting once builds are green. **An inclusive `files` list** naming what to package would be more explicit than a lone negation, and electron-builder's own docs argue against it: an inclusive list silently drops anything added later. The negation keeps the default and states only the exception. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> Reviewed against the five dimensions in `.github/instructions/code-review.instructions.md`. `npm run lint` clean. `npm test` — 780 passing, 0 failing. **1 [fix here] · 1 [follow-up] — the [fix here] was fixed before this PR was opened.** - 🟡 **architecture · [fix here]** — the first draft cleared `_logs` only before `save_cache`. That leaves the already-poisoned cache entry in place, since the save does not overwrite an existing key, so builds would have stayed red until the lockfile changed again. Moved the primary cleanup to just after `restore_cache`. - 🔵 **tests · [follow-up]** — no automated coverage that `vendor` stays out of the asar. A packaging assertion would need a build in CI rather than a unit test; noted in **Risks** above rather than filed, as it is one assertion on a directory that no longer exists in the app. Considered and cleared: no new dependency, no host binary, nothing crossing IPC, no spawn, no path composition, and `rm -rf` targets a literal quoted path set two lines above under `set -u`. The bash script runs on both the macOS and Windows agents and uses nothing platform-specific. </details> <details> <summary>Implementation notes</summary> The negation-only `files` behaviour is not folklore — `app-builder-lib/out/fileMatcher.js`: ```js if (!matcher.isSpecifiedAsEmptyArray && (matcher.isEmpty() || matcher.containsOnlyIgnore())) { customFirstPatterns.push("**/*") } ``` `containsOnlyIgnore()` is true when every pattern starts with `!`, so the default include is prepended and the exclusion is layered on top. The same function adds electron-builder's own exclusions in exactly the `!${dir}{,/**/*}` form used here. Local verification, on this branch versus without the `files` key, with a stand-in cache staged: | | `/vendor` entries in `app.asar` | total entries | |---|---|---| | without `files` | 6 | 28308 | | with `files` | 0 | 28302 | The 6-entry difference is exactly the staged cache. The byte difference is trivial here only because the stand-in cache is a few hundred bytes; on a CI agent it is the full package cache. </details> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
Stacked on #22 (macOS code signing). Review/merge that first — GitHub will retarget this to
trunkonce it lands.Rationale
The packaged app was ~1 GB on every platform. The renderer is esbuild-bundled at build time, so React,
@wordpress/*,@emotion/*, andxtermare already inlined intosrc/renderer— yet they also sat independencies, so electron-builder shipped their fullnode_modulestrees inside the app too.@xterm/xtermwas a dependency with zero references insrc(the renderer imports thextermpackage).Moving the build-only libs to
devDependenciesand dropping@xterm/xtermtrims the packaged production tree 1366 MB → 1196 MB (~170 MB, 12%), 492 → 312 packages.Not addressed here
The remaining bulk is
@php-wasm(~1 GB:@php-wasm/node654 MB +@php-wasm/web406 MB), pulled transitively by@wp-playground/cli. It ships every PHP version (7.2–8.4) in two execution strategies (asyncify + jspi), plus a browser build (@php-wasm/web) thatsrcnever imports directly. Cutting it needs a product decision on supported PHP versions/strategies and whether@php-wasm/webis reachable — left as a follow-up.How to test
electron-store(runtimeimport()), the main process,preload.js, and the spawned runner scripts were checked — none import the moved libraries.npm run build:oncebundles the renderer cleanly with the libs as devDependencies. Confirm the macOS/Windows/Linux builds still produce working apps and that artifact sizes drop by ~170 MB on the Buildkite run.