fix(pwa): harden service worker cache lifecycle - #187
Conversation
|
@AyobamiH is attempting to deploy a commit to the studentsuite Team on Vercel. A member of the Team first needs to authorize it. |
72a5d66 to
4bf0378
Compare
shauryagangrade
left a comment
There was a problem hiding this comment.
Review: SW cache lifecycle (#169)
First, the genuinely good parts — several real bugs fixed:
trimCachenow drains all excess instead of one entry per call. The oldif (keys.length > max) delete(keys[0])could never catch up under burst traffic → unbounded tile cache growth. Real fix.- Prefix-scoped cleanup with explicit legacy names (
app-studymap-v1/tiles-studymap-v1) stops the activate handler from deleting unrelated caches on a shared origin. Correct citizenship, and the rename bridge is covered. - Serialized tile mutations close the limit-check race;
/mapprecache is valid (route exists atsrc/app/map/page.tsx); registration errors surfaced instead of swallowed; testing the realsw.jsin avmcontext rather than reimplementing it is exactly right.
Blocking: the versioned script URL can't trigger reinstalls by itself
The stated goal — "a deploy activates a fresh cache namespace even when public/sw.js is unchanged" — doesn't hold per the service worker update algorithm. An update installs only when the fetched script's bytes differ from the current worker's cached bytes:
If
newestWorkeris not null and its cached bytes equal the fetched response body →HasNoChangedResource, abort.
The comparison is on response bytes only — query string differences are irrelevant (fragments have explicit handling in the spec; queries do not). So after this ships:
- Deploy N registers
/sw.js?v=shaN— installs fine. - Deploy N+1 (sw.js untouched) registers
/sw.js?v=shaN+1. Browser fetches it, bytes are identical to the current worker's cached script →HasNoChangedResource→ old worker stays. - The surviving worker's
self.location.hrefstill carriesv=shaN, soVERSIONkeeps resolving to the old namespace forever. Stale app-shell caches are never rotated — the exact failure mode #169 describes.
What ?v= does legitimately solve is HTTP-cache staleness of the script fetch itself — but Vercel already serves public/ assets revalidated, and it does nothing for byte identity.
Suggested fix
Make the bytes change every deploy instead of the URL — inject the build ID into the file at build time:
// scripts/build-sw.mjs — run in "prebuild"
const template = readFileSync("public/sw.template.js", "utf8");
writeFileSync("public/.sw-generated.js",
template.replace("__BUILD_ID__", process.env.VERCEL_GIT_COMMIT_SHA ?? ...));…then either register the generated path (/sw.generated.js, stable URL) or copy over public/sw.js before next build emits it. Either way VERSION becomes a plain baked-in const — no location.search parsing needed — and every deploy produces byte-different script → guaranteed install → activate rotates namespaces.
The existing tests survive unchanged apart from sourcing the generated file.
Happy to re-review quickly once the injection lands — everything else here is merge-ready.
|
@AyobamiH Please work on the changes requested above, we'll review them when you commit, and merge if everything's in order. :-) |
Signed-off-by: Ayobami Haastrup <47716486+AyobamiH@users.noreply.github.com>
4bf0378 to
301616c
Compare
|
Addressed the service-worker byte-identity blocker in 301616c. /sw.js is now generated during prebuild with the build ID baked into the worker source, while registration stays on the stable /sw.js URL. Added a regression proving different build IDs produce different worker bytes. Final validation is green: 117/117 tests, build, lint, typecheck and data validation. |
shauryagangrade
left a comment
There was a problem hiding this comment.
Re-review of d7f315c: the $\-pattern nit is resolved via a replacer function (template.replace(TOKEN, () => JSON.stringify(buildId))). Verified locally that build IDs containing $&, $`, $', $1 now render verbatim and produce valid JS (node --check); 4/4 service-worker lifecycle tests pass; eslint clean; CI green on head.
Note for transparency: this follow-up commit was pushed directly to the PR branch by a maintainer as part of the review cycle.
Merging.
Closes #169.
What changed
public/sw.jsduringprebuildfrom a service-worker template./sw.jsURL.studymap-app-*andstudymap-tiles-*cache namespaces so cleanup cannot match generic caches belonging to another application.app-studymap-v1andtiles-studymap-v1cache names explicitly during activation./mapto the app-shell precache.TILE_LIMIT.app-another-productandtiles-another-productsurvive StudyMap cleanup.Verification
Final commit:
301616c690686e23bb15f3c1dd16acbad3d8c544Validated against the current upstream base:
npm run buildpassednpm run lintpassednpx tsc --noEmitpassedThe production build also verifies that
prebuildgeneratespublic/sw.jswith the build ID embedded beforenext buildruns.Validation PR:
AyobamiH#7