fix(environments): keep local custom/ media out of every production build - #44
Merged
Merged
Conversation
…uild The strip plugin gated on AVATAR_SHIP=1, which only dist:win set, so `npm run build` and `npm run desktop` still expanded the eager glob over src/assets/environments/custom/ and hashed contributor trial media into dist/. Invert the default: anything that emits a bundle drops custom/, keyed on command === 'serve' so --mode cannot flip it, with AVATAR_INCLUDE_CUSTOM=1 as the deliberate opt-in. AVATAR_SHIP is removed. The rewrite also failed silently — String.replace returns its input when the pattern misses, and the plugin returned that as if it had worked, so renaming or reformatting the glob would have quietly restored the leak. It now errors and fails the build. Add scripts/custom-envs.test.mjs. A clean checkout has an empty custom/, so CI's production build stays green even with the plugin deleted; the build is not a regression test for this on its own. Five tests drive the real plugin over the real environments.js, and two plant a file and run an actual build, which is what catches media reaching the bundle by a route other than that one glob. Closes ARPAHLS#13 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…olds The build tests asserted only that the planted probes were absent, which leaves room for custom/ influencing the bundle without its files being copied — different chunking, different content hashes. Compare two builds populated with different probe sets instead, over a manifest of every emitted file and its md5, so the promise being checked is the one the docs make: what sits in custom/ makes no difference to a production bundle. Three builds shared across four assertions rather than one build each, so the stronger check costs one extra build. Probe stems now depend only on the set label: the opt-in build reuses the plain build's filenames, which leaves the flag as the only variable between them and makes the control fail when the embargo is broken (it did not before). Surface vite's stderr when a build fails, since stdio: 'pipe' otherwise leaves a CI failure here with nothing to go on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 tasks
Contributor
|
Thanks @AUDOSt0ck1ng, this looks solid. For tests I think it's ok, still not much time, but good point, maybe worth exploring follow up with node --test --test-name-pattern / separate test:build if CI time becomes an issue. Merging this as-is. Appreciate the thorough write-up and the mutation checks. <3 |
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
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.
Closes #13.
What
npm run buildandnpm run desktopbundled anything sitting inavatar/src/assets/environments/custom/. Onlydist:windid not. Now no build does, unless asked.Why
stripCustomEnvsForShiprewrites the eagerimport.meta.globinsrc/config/environments.jsto{}before Vite's import-analysis expands it, so custom media never enters the module graph. It gated onprocess.env.AVATAR_SHIP === '1', anddist:winwas the only script setting it — the other two fell through the first line oftransform.Inverted per the issue's preferred direction:
Keyed on
commandrather thanmode, so--modecan't flip the embargo.AVATAR_SHIPis removed rather than left as a dead flag. Opt in withnpx cross-env AVATAR_INCLUDE_CUSTOM=1 npm run build(cross-envbecause bareVAR=1 cmddoesn't work in PowerShell orcmd).Two things beyond the straight inversion:
String.replacereturns its input when the pattern misses, and the plugin returned that as if it had worked — so a rename or reformat of the glob would have quietly restored the leak. It now callsthis.error()and fails the build.npm testcovers the embargo. A clean checkout has an emptycustom/, sonpm run buildin CI stays green even with the plugin deleted outright. The build is not a regression test for this on its own — which is also why the bug survived and why a casual recheck makes it look already fixed.How tested
End to end, by hand (Windows), planting a 2 MB
PROBE_TEST.gifincustom/and running the realnpm run build:dist/assetsnpm run build(no match)npx cross-env AVATAR_INCLUDE_CUSTOM=1 npm run buildPROBE_TEST-ZgW6haIg.gif, 2,000,000 bytesThe opt-in row is the control. Without it, "found nothing" and "wasn't looking" are indistinguishable — which is the trap this bug sits in.
Bundle comparison. Built twice, once with
custom/empty and once holding 5.3 MB across three files, then diffed every emitted file by md5:So it is not just that the probes are absent; nothing in
custom/influences chunking or content hashes either. That comparison is nowscripts/custom-envs.test.mjs'sis byte-identical whatever custom/ holds, using two differently-sized probe sets so it works without assumingcustom/starts empty (a contributor's local trial media would break that assumption).Also verified:
vite build --mode developmentexcludes; the dev server still expands the glob (GET /src/config/environments.jsreturns theenvironments/custom/…paths); renaming the glob fails the build with a named error.Mutation-checked, to confirm the suite isn't vacuous:
pluginsincludeCustom = true(this bug reintroduced)import.meta.globforcustom/added insrc/main.jsxThe third is why the file isn't only transform-level assertions. Five tests check that the one glob in
environments.jsgets rewritten, and all five stay green when media reaches the bundle by another route; only the ones that plant real files and run real builds catch it.CI is green on my fork (Ubuntu, Node 22,
npm ci): lint, 35 tests, production build.npm run lint,npm test,npm run buildalso pass locally inavatar/.Behavior change
npm run desktopno longer showscustom/media, since it runs a production build. Usenpm run dev:desktop, or the opt-in. Documented inREADME.md,CONTRIBUTING.md,docs/environments.md,docs/development/project-layout.md.Not verified
dist:winis Windows-only and not in CI).electron-builderpacksdist/**verbatim per thebuild.filesfield, so a cleandist/implies a clean installer — inferred, not run.dist/, the Custom expander has nothing to render. Note it also lists runtime entries from Settings → Directories, which this change does not touch.docs/development/project-layout.md; happy to trim if that's unwelcome innpm test.🤖 Generated with Claude Code