Skip to content

Stop shipping build-only deps in the app - #23

Draft
mokagio wants to merge 5 commits into
trunkfrom
mokagio/trim-desktop-artifact-deps
Draft

mokagio wants to merge 5 commits into
trunkfrom
mokagio/trim-desktop-artifact-deps

Conversation

@mokagio

@mokagio mokagio commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #22 (macOS code signing). Review/merge that first — GitHub will retarget this to trunk once it lands.

Rationale

The packaged app was ~1 GB on every platform. The renderer is esbuild-bundled at build time, so React, @wordpress/*, @emotion/*, and xterm are already inlined into src/renderer — yet they also sat in dependencies, so electron-builder shipped their full node_modules trees inside the app too. @xterm/xterm was a dependency with zero references in src (the renderer imports the xterm package).

Moving the build-only libs to devDependencies and dropping @xterm/xterm trims 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/node 654 MB + @php-wasm/web 406 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) that src never imports directly. Cutting it needs a product decision on supported PHP versions/strategies and whether @php-wasm/web is reachable — left as a follow-up.

How to test

electron-store (runtime import()), the main process, preload.js, and the spawned runner scripts were checked — none import the moved libraries. npm run build:once bundles 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.

@mokagio mokagio self-assigned this Jul 3, 2026
@mokagio
mokagio changed the base branch from ainfra-2597-fix-macos-code-signing-for-contributor-toolkit to trunk July 10, 2026 11:05
@mokagio
mokagio force-pushed the mokagio/trim-desktop-artifact-deps branch from b6ae55b to 1f8ca57 Compare July 10, 2026 11:05
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>
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>
mokagio and others added 5 commits August 21, 2026 11:06
Probe for AINFRA-2597: electron-builder reports the Developer ID
identity as `CSSMERR_TP_NOT_TRUSTED`, so the macOS build ships unsigned.
The cert installs fine — the trust chain fails to evaluate — so dump
keychain search list, `find-identity` output, intermediate-CA presence,
and an explicit `verify-cert` on the CI agent to pinpoint the missing
anchor before committing to a fix. Non-fatal; reverted once diagnosed.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The macOS 26 CI image ships no "Developer ID Certification Authority"
G2 intermediate, and `match` installs only the leaf. With no keychain
in the search list able to build `Developer ID App → Developer ID CA G2
→ Apple Root`, `security` evaluates the identity as
`CSSMERR_TP_NOT_TRUSTED` and electron-builder silently skips signing —
the same failure and fix as the WordPress desktop (calypso) app.

Fetch and import the intermediate into the temp keychain `setup_ci`
creates, gated to CI. Add a `verify_code_signing` lane
(`codesign --verify` + Gatekeeper `spctl --assess` + Developer ID
authority assertion) and run it after `npm run dist`, so an unsigned or
mis-signed build fails the job instead of shipping.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI build #72 confirmed the trust chain now resolves (`verify-cert`
succeeds, `find-identity` reports a valid identity) and the app is
signed, notarized, and Gatekeeper-accepted. The probe has served its
purpose; the permanent `verify_code_signing` lane guards the invariant
from here.

---

Generated with the help of Claude Code, https://claude.ai/code

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
mokagio force-pushed the mokagio/trim-desktop-artifact-deps branch from 1f8ca57 to 354058e Compare August 21, 2026 01:15
zaerl added a commit that referenced this pull request Sep 30, 2026
… system redesign (#563)

## Why

The redesign in #542 is built on `@wordpress/ui` and `@wordpress/theme`,
and both are developed against the current `@wordpress/components`. The
app is on 28, thirteen majors behind, so nothing from the design system
can be added until the renderer stands on 41. This is the first step of
#542 and changes no behaviour.

## What changes

- `@wordpress/components` moves from `^28.2.0` to `^41.0.0` and
`@wordpress/icons` from `^10.7.0` to `^17.0.0`. React stays on 18.3.1,
which both packages support.
- The `edit` icon was removed from `@wordpress/icons`; `pencil` is the
same glyph under its current name.
- `build.files` excludes the thirteen packages the update brought into
the packaged app. `@wordpress/components` 41 depends on `@wordpress/ui`
and `@wordpress/theme`, and `@wordpress/theme` names esbuild as an
optional peer, so npm stopped marking esbuild as dev-only and
electron-builder started shipping its binary. None of the thirteen is
loaded at run time: the renderer is the esbuild bundle, and nothing
under `src/` requires them.
- The sidebar title is kept on one line. Buttons are set in a heavier
weight now, and the wider "Collapse" button wrapped "Contributor
Toolkit" onto two.
- Text inputs are now 40px tall instead of 36px. So the buttons are
slightly smaller. But they will be removed anyway.

Deliberately not in this PR: adding `@wordpress/ui` or
`@wordpress/theme` as dependencies of the app, any use of design tokens,
and any layout change. Those are the next steps of #542.

## How to test this

Platforms: any. Nothing here touches paths, spawning or signing. Use the
Buildkite build for the current head of this branch.

The journeys carry the behavior: `npm run test:e2e` from the repository
root drives every flow the renderer has through the updated components,
and `npm run test:e2e:packaged`, after
`CSC_IDENTITY_AUTO_DISCOVERY=false npm run pack:dir`, proves the
packaged app still boots without the excluded packages. What they cannot
see is how the app looks, so that is what a person should check.

**Starting state:** the app open with at least one site that has
finished setup.

1. Look at the sidebar. "Contributor Toolkit" is on one line, beside
"Collapse".
2. Click "Collapse", then the arrow to expand again. The sidebar returns
to the same layout.
3. Click the pencil beside the site name. The "Rename site" dialog
opens; close it.
4. Open the menu at the top right of the site. It lists "Copy path", the
file manager entry, "Update to latest trunk" and "Delete this site".
5. Click "Create a site". The dialog shows "Site name", "Contribute to"
with two choices, and "Site location"; close it.
6. Scroll to "Logs" and click each tab. The selected tab is underlined
and its content changes.

**What must not have happened:**

- No button, field or menu has lost its label or become unreachable.
- No text is clipped or overlapping where a control grew: text fields
are 40px tall instead of 36px, and button labels are heavier.

## Risks and limitations

- **The look changes slightly, and that is the design system's doing.**
Button labels are heavier, text fields are taller, menu items are more
compact, and the selected tab is marked in neutral grey and black where
it was blue. Details under Screenshots.
- **The docs screenshots are not retaken.** The twelve the harness can
produce differ slightly from the app after this. #542 retakes all of
them, and the harness is not reproducible across runs (#326), so
refreshing them here would add churn twice.
- **Checked on macOS only by hand and by local runs.** Windows is
covered by CI's journeys and packaged smoke, not by a person.
- **The exclusion list is by name.** A future update that adds another
renderer-only package will ship it until someone adds a line. Excluding
the whole renderer-only tree is the real fix and overlaps with #23.
- **The first commit of this branch was pushed to `trunk` by mistake and
removed again.** `trunk` is back at 0dc54da; anyone who fetched in
between has b6a6151 on their local `trunk`.
- Review: 2 [fix here], both fixed; 1 [follow-up] deferred.

## Related

Part of #542.
Fixes #548.

---

<details>
<summary>Design decisions and alternatives considered</summary>

**Excluding in `build.files` rather than moving the renderer's
dependencies to `devDependencies`.** Moving them would stop the whole
renderer-only tree from shipping, not just what this update added, but
that is a change to how the app is packaged and #23 is already about it.
This PR only undoes the growth it would otherwise cause.

**One jump from 28 to 41 rather than a major at a time.** The renderer
uses seventeen components, all of which still exist with the same props.
The only break was the removed icon alias, which fails the build rather
than the app.

</details>

<details>
<summary>Review outcome (required — see AGENTS.md)</summary>

**2 [fix here] · 1 [follow-up]** — both [fix here] fixed.

- Architecture, medium, [fix here], fixed: the packaged app shipped the
esbuild binary, because `@wordpress/theme` names esbuild as an optional
peer and electron-builder collects optional packages.
- Performance, medium, [fix here], fixed: twelve more packages the
update added were collected into the app and never loaded.
- Performance, low, [follow-up], deferred: the whole renderer dependency
tree ships although only the bundle is loaded. It predates this branch
and is the subject of #23.

Style notes, not applied: with `white-space: nowrap` the sidebar title
can paint outside the sidebar during the 200ms expand animation; the
`esbuild@0.23.1` entry in `allowScripts` has been stale since #538.

- **Review:** completed — fresh agent context, local, before the PR
existed; head b6a6151 / base
0dc54da; committed work only, working
tree clean; outcome above. CodeRabbit: not run yet.
- **Since review:** `b6a6151 → 4b2d52e` checked. The difference is the
thirteen exclusion lines in `build.files` that fix the two findings. It
was not reviewed again by a fresh context; it was verified by packaging
and running the packaged smoke test.

Checks on 4b2d52e, macOS, local:

| Check | Result |
|---|---|
| `npm run lint` | clean |
| `npm test` | 1676 tests, 1674 pass, 2 skipped, 0 fail |
| `npm run test:electron` | 1676 pass, 0 fail |
| `npm run test:e2e` | 33 of 33 pass |
| `npm run test:e2e:packaged` | 15 of 15 pass |

Lint, the unit tests and the journeys ran on b6a6151; the second commit
changes `build.files` only, which none of the three reads.

</details>

<details>
<summary>Implementation notes</summary>

`app.asar` on macOS arm64, unsigned, `npm run pack:dir`:

| State | Size |
|---|---|
| After the update, before the exclusions | 238,010,661 bytes |
| With the exclusions | 199,873,553 bytes |

The size on `trunk` was not measured.

Packages excluded: `esbuild`, `@esbuild/*`, `@base-ui/*`, `@date-fns/*`,
`@daypicker/*`, `react-day-picker`, `reselect`, `tabbable`,
`colorjs.io`, `@wordpress/base-styles`, `@wordpress/kebab-case`,
`@wordpress/style-runtime`, `@wordpress/ui`. The list is the difference
between the two lockfiles in what is not marked dev-only.

`@wordpress/components` 41 injects part of its CSS at run time through
`@wordpress/style-runtime`. The renderer's content security policy
already allows it (`style-src 'self' 'unsafe-inline'`).

</details>

<details>
<summary>Screenshots or recording</summary>

No images attached. The comparison was made between the committed docs
screenshots and the same twelve shots captured on this branch with `npm
run shots`. What differs:

- Button labels are set in weight 500.
- Text fields are 40px tall instead of 36px.
- Menu items are more compact.
- In "Logs", the selected tab has black text and a grey underline; it
was blue.
- The sidebar title wrapped onto two lines before the fix in this PR,
and is on one line with it.

</details>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
@ryanwelcher

Copy link
Copy Markdown
Collaborator

@mokagio is the PR something you'll be able to move forward on?

zaerl added a commit that referenced this pull request Oct 23, 2026
…braries and Chromium locales (#707)

## Why

The packaged app is about 920 MB on disk on macOS (320 MB zipped), and
about a third of that is code it never loads. That is a long download on
a Contributor Day network, where everyone fetches the app at the same
time. It is the same problem #23 reported. That PR has since gone stale
against trunk, and its claim that `@xterm/xterm` is unused no longer
holds, so this branch starts again from trunk.

## What changes

Three things the app ships and never loads are left out:

- **The asyncify PHP builds, about 220 MB.** Each `@php-wasm/node-*`
package carries every PHP version twice, as `jspi/` and as `asyncify/`,
and `@php-wasm/node` reads `asyncify/` only when `"Suspending" in
WebAssembly` is false. Electron 43.6.0 has JSPI, so those folders never
load. `@php-wasm/node-5-2` goes too, because the app offers only
`SupportedPHPVersions` (7.4–8.5). Both are `build.files` exclusions;
`node_modules` is untouched.
- **The renderer's libraries, about 95 MB in `app.asar`.** esbuild
already bundles React, Emotion, xterm and the `@wordpress` UI packages
into `src/renderer/index.js`, but they were listed in `dependencies`, so
electron-builder copied their trees in as well. They move to
`devDependencies`. `@wordpress/i18n` and `@wordpress/hooks` stay,
because the main process loads them. The 22 hand-kept `build.files`
exclusions that trimmed part of that tree since #563 are now unnecessary
and are removed.
- **Chromium's locales for languages the app has no catalog for, about
43 MB.** The app shows English for those anyway, so
`build.electronLanguages` keeps English and the Chromium locale of each
catalog in `src/languages`. **`npm run i18n:download` now writes that
list** whenever it adds or removes a catalog, so the version-bump PR
carries the change with no manual step. `scripts/chromium-locales.cjs`
maps a catalog to the locale Chromium itself would load, including
Chromium's regional folding (`es-mx` → `es-419`, `pt` → `pt-PT`, `zh-hk`
→ `zh-TW`, `en-ca` → `en-GB`). Every regional locale is written in both
spellings, `pt-BR` for Windows and Linux and `pt_BR` for macOS, because
electron-builder matches file names.

Unit tests guard the cuts:

- `electron-wasm-jspi` asks the bundled Electron for
`WebAssembly.Suspending`, so an Electron bump that loses JSPI fails CI
instead of the first dev server start.
- `electron-languages` asserts that the list equals the one the catalogs
call for, which catches both a missing entry and a stale one. It also
checks that electron-builder keeps every locale file the list names.
- `chromium-locales` covers the mapping with both platforms' locale
names.
- `php-builds` fails when an installed `@php-wasm/node-X-Y` whose
version `SupportedPHPVersions` does not offer would ship, and when a
version the settings offer is excluded.

Run against translate.wordpress.org today, the script adds `sr` and `tr`
for two catalogs that are above the cut-off but not yet in the
repository, and leaves out `lo`, which Chromium has no locale for.

Not in this PR: dropping PHP versions the settings offer (a product
decision since #559), and the `intl` extension and ICU data, which
core's tests use.

| macOS arm64, unsigned, same `node_modules` | trunk | this branch |
|---|---|---|
| `.app` on disk | 920 MB | **571 MB** |
| zipped | 320 MB | **218 MB** |
| `app.asar` | 205 MB | 108 MB |
| `app.asar.unpacked` | 436 MB | 228 MB |
| Electron Framework | 277 MB | 234 MB |

## How to test this

Platforms: macOS and Windows, both needed, because the PHP runtime is
chosen when the server starts and no CI job starts the real Playground
server (see the Electron pin note in AGENTS.md). Use the Buildkite
artifact for the current head commit.

**Starting state:** the app from this branch's Buildkite build, with a
site that has finished setup and whose server is stopped.

1. Open the site and click **Start development server**. The server
comes up, and the site's front page and `/wp-admin/` both load in the
browser.
2. Open the app's log for that site. It has no `Cannot find module`
error, and no path containing `asyncify`.
3. In the settings, change the PHP version to 7.4, then start the server
again. The site loads, and the log names PHP 7.4.
4. Use the app as usual: the site list, the terminal (xterm), the
changed files, a dialog. Everything draws as before. The renderer no
longer reaches `node_modules` for any of it.
5. On macOS, compare the size of the app with a trunk build in Finder
(Get Info): about 570 MB against about 920 MB.

**What must not have happened:**

- The dev server failing to start, or starting on a PHP version other
than the one chosen.
- A blank or unstyled window, which would mean the renderer still needed
a package that is no longer shipped.
- Native form strings, such as a validation bubble, in a different
language from before for cs, hu, nl, pl or English.

## Risks and limitations

- The asyncify cut depends on Electron keeping JSPI. That is what
`electron-wasm-jspi` guards.
- On macOS, electron-builder also removes the grammatical-gender
overlays (`cs_FEMININE.lproj` and similar) for the kept languages; each
holds an 18-byte `locale.pak`. Chromium should fall back to the base
locale, but I have not checked a macOS user with a grammatical gender
set.
- `electron-languages` copies electron-builder's matching rule, because
it is not exported. An electron-builder release that changes that rule
would not be caught.
- Chromium's regional folding rules in `scripts/chromium-locales.cjs`
follow `CheckAndResolveLocale` in Chromium's `l10n_util.cc`, but no
Chromium source is vendored here to check them against. Only `en-ca` →
`en-GB` affects today's list, and `en-GB` is kept for `en-gb` anyway.
- `npm run i18n:download` now needs an installed Electron, which it
downloads on first use. It reads Electron's locales before changing any
catalog, so a failed download leaves everything as it was.
- The packaged smoke suite passed locally only with `--user-data-dir`
added to its launch, because a dev copy of the app held the
single-instance lock. Trunk's build failed the same way without it. CI's
run on this PR is the unmodified one.
- Review: one completed fresh-context review, 0 findings; details below.

## Related

- Fixes #703: the `asyncify/` folder of every PHP build is left out,
which is the exclusion the issue proposes, and `electron-wasm-jspi`
guards the JSPI it depends on.
- Fixes #704: `@php-wasm/node-5-2`, the only installed build whose
version `SupportedPHPVersions` does not report, is left out.
`php-builds` fails when a Playground update changes which builds those
are, and names the exclusion to add or remove.
- Fixes #705: `build.electronLanguages` keeps only English and the
Chromium locales of the app's catalogs, and `npm run i18n:download`
keeps it in step.
- Follow-up to #23, which reported the renderer-library half. That PR
belongs to its author and is left as it is.

---

<details>
<summary>Design decisions and alternatives considered</summary>

- **Exclusions in `build.files`, not a `postinstall` prune.**
`node_modules` stays complete for development, the tests and the
journeys. Only what electron-builder copies changes.
- **`devDependencies` rather than more `!node_modules/...` lines.** The
exclusion list from #563 had to grow every time a `@wordpress` package
gained a dependency. Moving the cause leaves nothing to maintain, and
lets 22 lines go.
- **A hand-written `electronLanguages` list with a test, rather than one
generated at build time.** A generated list would need a JS build config
or a hook; a static list plus a test that names the missing entry is
simpler, and changes only when a catalog is added.
- **Chromium locales for the app's catalogs only.** Keeping a broader
set would leave Chromium's strings in, say, Italian around an English
app, which is no better than English.

</details>

<details>
<summary>Review outcome (required — see AGENTS.md)</summary>

0 [fix here] · 0 [follow-up]. One style note was applied: the
`electron-languages` header claimed Chromium's strings follow the app's
language, but they follow the operating system's.

- **Review:** completed. Reviewer: a separate Claude agent context,
given only the diff and
`.github/instructions/code-review.instructions.md`. Reviewed head
`a7be033` against base `2dda92c` (`origin/trunk`). Evidence: `npm run
lint` passed; `npm test` passed with 2202 tests, 2200 pass, 0 fail, 2
skipped; `npm run test:electron` passed with 2202 tests, 2201 pass, 0
fail, 1 skipped. The five dimensions found nothing. Specifically
checked: no main-process, preload or runner import of a moved package;
no production dependency still reaching a formerly excluded package;
asyncify reached only when JSPI is absent, including for intl and
xdebug; PHP 5.2 unreachable from the settings; electronLanguages against
electron-builder's matching rule and the app's locale resolution.
- **Since review:** `a7be033 → 7ac9e25` checked. The only change is the
comment wording in `tests/unit/electron-languages.test.cjs`, and that
test still passes.
- **Since review:** `7ac9e25 → 8e1d005`, a CI fix, re-checked by the
author against the same standard: 0 findings. CI's unit job failed
because Electron 43's npm package downloads its binary on the first
`require('electron')`, and three test files now asked for it at the same
moment. I reproduced that locally by removing
`node_modules/electron/dist`: two downloads ran and two tests failed. A
`pretest` script now asks once before `node --test` starts, and the
workflow comment that said `npm ci` fetches Electron is corrected. Run
from a missing binary, `npm test` downloads once and passes 2200 tests
with 0 failures, and `npm run test:electron` and `npm run lint` pass
too.
- **Review of `8e1d005 → dde0f94`** (the `i18n:download` change):
completed by a separate Claude agent context against the same standard,
in the context of `origin/trunk...dde0f94`. Result: 1 [fix here] · 0
[follow-up], fixed in `26e9250`. The finding: Electron's locale names
were read after the catalogs had been replaced, so a failed Electron
download would leave the run half applied. They are now read before
anything is written. One style note was applied as well, a test title
that claimed more than it asserted. The reviewer also ran about 130
slugs through the mapping without a crash, and checked that rewriting
`package.json` reproduces the file byte for byte. Lint passes, and `npm
test` and `npm run test:electron` each pass 2212 tests with 0 failures.
- **Since that review:** `dde0f94 → 26e9250`. Only that fix and the
renamed test changed; the suites above ran on `26e9250`.
- **Since that review:** `26e9250 → 55ab855` adds only
`tests/unit/php-builds.test.cjs`, for #704. I checked it against the
standard myself: 0 findings. Broken both ways in turn (5.2 kept, 8.3
excluded), it fails each time, and it passes as committed. `npm test`
and `npm run test:electron` each pass 2214 tests with 0 failures.
- **CodeRabbit:** not requested yet.

</details>

<details>
<summary>Implementation notes</summary>

- Sizes come from `electron-builder --dir` with `-c.mac.identity=null
-c.mac.notarize=false`, run twice from the same `node_modules` with
trunk's and this branch's `package.json` and lockfile, then zipped with
`ditto -c -k`. Signed artifacts will differ slightly.
- Locally, from the trimmed package, `@php-wasm/node` booted PHP 7.4.33,
8.3.33 and 8.5.10 with `intl` loaded, run on the packaged binary with
`ELECTRON_RUN_AS_NODE=1`.
- `npm run test:e2e:packaged` passed all 15 tests on the trimmed build,
including the first window painting, the preload bridge, and the
`build.files` allow-list checks.
- In the lockfile, the moved packages and their trees change from
`devOptional` to `dev`. No versions change.
- `pretest` runs `node -e "require('electron')"`. The electron package
downloads its binary on the first `require`, so this fetches it once
before `node --test` runs its files in parallel, where concurrent
downloads raced. `npm run test:electron` already did this in its runner,
and a single file run with `node --test <file>` has nothing to race
with.
- The asyncify choice is made in `@php-wasm/node/index.cjs`
(`loadNodeRuntime`) and in each `@php-wasm/node-X-Y/index.cjs`
(`getPHPLoaderModule` and the extension paths).

</details>

<details>
<summary>Screenshots or recording</summary>

Nothing on screen changes.

</details>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants