feat!: default SPA fallback files off, add --fallback opt-in - #1
Merged
Conversation
Clears the one moderate Dependabot alert: @opentelemetry/core GHSA-8988-4f7v-96qf (unbounded memory allocation in W3C Baggage propagation), which reached us four levels down as bulletin-deploy -> @sentry/node@9 -> @opentelemetry/core@1.30.1. 0.14.2 moves @sentry/node ^9.14.0 -> ^10.69.0, which pulls @opentelemetry/core 2.10.0 (>= the patched 2.8.0). Fixing it upstream beats an npm override -- no forced resolution to carry. The pin stays exact. src/index.ts's pre-flight DotNS label validation encodes which versions rewrite vs refuse non-compliant labels, so the sanitizeDomainLabel / assertLabelIsPopRulesSafe region was diffed across the two versions: byte-identical, no source or comment change needed. bulletin-deploy's bin map is also unchanged, so cli.ts's manifest-based resolution still finds it. Gate: tsc clean, 35/35 vitest, `npm audit --omit=dev` at 0 vulnerabilities (was 1 moderate), and a --dry-run confirms resolution now reports bulletin-deploy@0.14.2.
The spawn line was the only unexercised code in the package: --dry-run proves staging and dependency resolution, but stopped one statement short of the handoff, so argv construction, output inheritance, exit-code propagation and staging cleanup had no coverage. Uses a stub child rather than a real deploy -- a real one needs a funded signer and writes a DotNS name on-chain. `BULLETIN_DEPLOY_BIN` is the seam for it; the stub records the argv it was handed and exits with a code the test chooses, which is the whole contract cli.ts relies on. One case deliberately omits the override and stops at --dry-run, so the pinned-dependency resolution path is covered too. Two environmental details keep it deterministic: the CLI is compiled to a throwaway outDir under node_modules/ (CI runs `npm test` before `npm run build`, so dist/ may not exist -- verified green with dist/ moved aside -- and building into it would turn the later build step into a no-op), and a stub `ipfs` goes on PATH since the Kubo preflight is deliberately fatal and runners lack Kubo. Falsified against four mutations of cli.ts: swapped argv order, swallowed exit code, dry-run no longer short-circuiting, and staging never cleaned up. Each was caught by the case written for it. The argv swap fails 7 cases rather than 1, because the stub reads its first argument as a directory and writes no record when that throws; the other three fail only their own case. The dry-run mutation also failed the pinned-resolution case by actually invoking the real binary, which is why the spawn carries a timeout. 46 tests pass, tsc clean.
`stageArchive` wrote 404.html as a byte-identical copy of index.html plus a tiny _redirects, but neither the Polkadot app sandbox nor polkadot-desktop reads either file (see the README's "Deep links do not work" section) — they only matter on a plain Kubo gateway. The duplicate is not free: it doubles the bytes bulletin-deploy has to chunk and upload. Measured on a live deploy, a 1,708,272-byte single-file app produced 19 on-chain chunks (nonces 37211-37228) because of the duplicated payload, and the duplicate blocks carry identical CIDs and still get uploaded twice. It compounds with paritytech/bulletin-deploy#1233: packSection fragments files in the 1-2 MB band into one chunk per 256 KB IPFS block, so for a file in that band the duplicate is +100% of an already-inflated chunk count. BREAKING CHANGE: parseArgs now defaults `fallback` to false, so `decentralize` no longer writes 404.html/_redirects unless --fallback is passed. --no-fallback is kept as a recognised, explicit no-op (rather than removed) so it does not fall through to the passthrough-forwarding branch and silently reach bulletin-deploy as an unrecognised flag — existing scripts and CI invocations that still pass --no-fallback keep working unchanged.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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.
Note on this diff's base:
origin/mainis currentlyfa6fff3and does not yet include two commits that exist locally onmainin this repo's primary checkout:11bda9b(bulletin-deploy 0.14.1 → 0.14.2) and0564886(addssrc/cli.test.ts, the end-to-end handoff coverage this PR builds on and extends). This branch is based on0564886rather thanorigin/mainso the new tests could go through a real red/green TDD cycle against the existingcli.test.tsharness instead of a freshly-invented one. Oncemainis pushed with those two commits, this diff will narrow to just the fallback-default change. Not something I'm merging or pushing tomainmyself — flagging it so the diff below doesn't look like it inventedcli.test.tsfrom scratch.Why
stageArchivewrote404.htmlas a byte-identical copy ofindex.html, plus a tiny_redirects. Neither file is read by the Polkadot app sandbox nor by polkadot-desktop — the README already documented this at the end of its "Deep links do not work" section. They only matter on a plain Kubo gateway.The cost is real and was measured on a live deploy: a 1,708,272-byte single-file HTML app produced 19 on-chain chunks (nonces 37211–37228), because the duplicate
404.htmldoubled the payload. The duplicate blocks even carry identical CIDs and are still uploaded twice.It compounds with an upstream bug filed as paritytech/bulletin-deploy#1233:
packSectionfragments files in the 1 MB–2 MB band into one chunk per 256 KB IPFS block (2.0 MB → 8 chunks, 2.2 MB → 2 chunks). So the duplicate copy is not merely +100% — for files in that band it is +100% of an already-inflated chunk count.Writing a byte-identical duplicate of the payload is the wrong default for the primary deployment target. Opt in if you actually serve from a plain Kubo gateway.
What changed
parseArgsinsrc/index.tsnow defaultsfallbacktofalse(wastrue).--fallbackflag opts back in (today's old default behavior: writes404.html+_redirects).--no-fallbackremains a recognised flag, accepted as an explicit no-op, with a code comment explaining why:decentralizeforwards every unrecognised flag verbatim tobulletin-deploy, so removing the case would turn--no-fallbackinto a silent passthrough argument and break any existing script or CI invocation that still passes it.stageArchive's signature is unchanged — it already tookfallback: booleanand keeps doing exactly what it's told; only the CLI's default changed.printUsage()and the README (options table, "What it actually does" list, and the caveat paragraph) all updated to reflect the new default and reference bulletin-deploy#1233.BREAKING CHANGE
The CLI's default behavior changes:
decentralize <dir> --dot <name>no longer writes404.html/_redirectsunless--fallbackis passed explicitly.--no-fallbackis deliberately still accepted as a no-op so existing invocations keep working without modification.Test plan
TDD: wrote/adjusted failing tests first, watched them fail (4 failing — pasted below), then implemented.
Red phase:
Green phase (after implementing):
parseArgs([...])default now yieldsfallback: false--fallbackyieldsfallback: true--no-fallbackis not forwarded intopassthrough(regression test, including the lookahead-swallow case)--fallbackis likewise not forwarded intopassthroughsrc/cli.test.tsend-to-end: default run stages without404.html/_redirects,--fallbackstages with them--no-fallbackparses-to-falsecoverage kept (still proves it parses, no longer proves it changes anything on its own)stageArchive-level "writes the fallback files only when asked" coverage kept unchanged