Refreshless website deployments – load remote.html using the network-first strategy #1849
Conversation
adamziel
changed the title
adamziel
force-pushed
the
fix-service-worker-updates
branch
from
b3a49e3 to
18af253
Compare
Are there any dynamically requested resources that are dependencies of a given remote.html dependency tree? If so, I wonder whether we could somehow track whether remote.html clients depend upon a specific cache and only remove an offline cache once no more clients depend upon it. |
PHP.js and zipped WordPress versions for sure, perhaps there's a few more.
I would love a flavor of that where we only remove those cached entries that are no longer present in the new version. For example, if the |
| * There's still a small window of time between loading the remote.html file and | ||
| * fetching the new assets when a new deployment would break the application. | ||
| * This should be very rare, but when it happens we provide an error message asking | ||
| * the user to reload the page. |
There was a problem hiding this comment.
Lovely, informative comments here.
| @@ -105,7 +113,8 @@ import { wordPressRewriteRules } from '@wp-playground/wordpress'; | |||
| import { reportServiceWorkerMetrics } from '@php-wasm/logger'; | |||
|
|
|||
| import { | |||
| cachedFetch, | |||
| cacheFirstFetch, | |||
| networkFirstFetch, | |||
There was a problem hiding this comment.
Solid naming. Clear and explicit. ✊ ❤️
| * have the previous version cached in CacheStorage. | ||
| * | ||
| * This very simple resolution took multiple iterations to get right. See | ||
| * https://github.com/WordPress/wordpress-playground/issues/1821 for more |
| } | ||
| } | ||
|
|
||
| const registration = await sw.register(serviceWorkerUrl + '', { |
There was a problem hiding this comment.
Is that concat just for converting to string? I figured the API would take URL objects but haven't checked.
The spec isn't super straightforward to read on this IMO, but the MS TypeScript type seems to suggest URL is a supported type for that param:
https://github.com/microsoft/TypeScript/blob/b845fd2434f255ba35b0bb143a65172c8985e467/src/lib/webworker.generated.d.ts#L5425
| // Proxy the service worker messages to the web worker: | ||
| navigator.serviceWorker.addEventListener( | ||
| 'message', | ||
| async function onMessage(event) { |
There was a problem hiding this comment.
It's cool how you flattened register-service-worker.ts into this module. I was never sure why it was separated, but it was never important enough to pursue.
| maxDiffPixels, | ||
| }); | ||
|
|
||
| await expect(website.page.getByLabel('Open Site Manager')).toBeVisible(); |
There was a problem hiding this comment.
Why move away from screenshots? I can think of reasons (e.g., test fragility) but am curious about yours.
Motivation for the change, related issues
Related to #1821
Changes the webapp upgrade protocol proposed in #1822 to avoid forcibly refreshing the browser tabs with unsaved changes in them.
Technical implementation
Before this PR, the new service worker would clear the offline cache, claim all the active clients, and forcibly refresh them to ensure the latest Playground version is loaded everywhere.
This worked, but every webapp upgrade would destroy any work the user may have done in their temporary Playground. We've explored storing temporary Playgrounds in OPFS, but backtracked because 1) it created an uncanny amount of complexity, and 2) some browsers (e.g. Safari in private mode) don't support OPFS and must rely on a temporary in-memory site.
After this PR, the service worker clears the offline cache, claims all the active clients, but it doesn't forcibly refresh them. Instead, it uses the network-first strategy for the
remote.htmlroute and the/route. All the other files are still loaded using the cache-first strategy.Every Playground that's already open, either temporary or stored, will remain functional. The heavy, asynchronously loaded resources such as PHP.wasm and WordPress.zip were already processed – there's no user flow that could lead to
import()-ing a non-existingphp.jsfile.Every newly opened Playground will be loaded using a freshly downloaded
remote.htmlfile containing references to freshly deployed Playground assets. ThusOther changes
This PR inlines the reusable service worker utilities from
packages/php-wasm/web/src/lib/register-service-worker.tsinto@wp-playgroundpackages. It turns out, they weren't as reusable and keeping them separate was annoying. I'm now convinced the service worker bits are application specific and splitting them between multiple packages just isn't useful.Testing instructions
Review the app deployment E2E tests check what we need to check, and them confirm they are green in the CI.