Repository navigation
fix: serve precached files when network-first requests fail offline - #26624
totally-not-ai[bot] wants to merge 6 commits into
Conversation
In development mode with a dev bundle, the service worker routes /VAADIN/* through a network-first strategy that is registered before the precache route. When offline, that strategy only looks in the runtime cache, which is empty until the service worker has handled a request once. The dev bundle scripts are precached, but were never served from the precache, so reloading offline after the first visit left a blank page instead of the offline page. Fall back to the precache when a network-first request fails.
Type of change
How to test
|
Add a dev bundle variant of the PWA test module, and a test that fetches the precached bundle files offline without fetching them online first, so that only the precache can serve them.
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 1 comment.
| Finding | |
|---|---|
| 🧪 | The only test that exercises the fix can silently skip itself. |
| // The dev bundle and the production bundle are precached, while the | ||
| // frontend served by the Vite dev server is not | ||
| List<String> bundleUrls = getPrecachedBundleUrls(); | ||
| Assume.assumeFalse("No precached bundle files", bundleUrls.isEmpty()); |
There was a problem hiding this comment.
🧪 The only test that exercises the fix can silently skip itself.
testPrecachedBundleOffline is the sole guard for this change: it is skipped in hotdeploy and passes in production with or without the fix, so only the new dev-bundle module proves anything. The skip condition is derived from the very thing being measured — if /VAADIN/build/ ever stops matching the precache keys (assets dir renamed, manifest injection broken), or the inline script throws and hits .catch(() => resolve([])), the test turns green everywhere and the fix loses its coverage with no signal.
Keying the skip on the configuration (hotdeploy) rather than on an empty precache would keep it honest.
PwaTestIT.java:250 · tests
There was a problem hiding this comment.
Good catch, fixed in 877d934. The test now skips based on the configuration: test-pwa/pom.xml passes vaadin.frontend.hotdeploy to failsafe, and the test skips only when that property is true. The dev bundle and production variants don't set it, so there an empty precache now fails with "Expected precached bundle files" instead of skipping. The inline script also no longer turns errors into []. It returns the error text, and the test fails with "Reading the precache failed: …". Verified: skipped with hotdeploy, passes in production and with the dev bundle. Against a flow-server without the fix, the dev bundle run still fails as expected.
Skipping when no precached bundle files were found let the test pass silently if the precache stopped matching or could not be read. Skip on the hotdeploy configuration instead, and fail when the precache is empty or reading it throws. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
📦 Snapshot published
Built without running tests. Look at the checks on this pull request before relying on it. Every new commit on this branch republishes it while the How to use it<repository>
<id>vaadin-snapshots</id>
<url>https://maven.vaadin.com/vaadin-prereleases</url>
<snapshots><enabled>true</enabled></snapshots>
</repository>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>com.vaadin</groupId>
<artifactId>flow-bom</artifactId>
<version>25.4.sw-network-first-precache-fallback-SNAPSHOT</version>
<type>pom</type>
<scope>import</scope>
</dependency>
<!-- After flow-bom, so that its Flow version wins -->
<dependency>
<groupId>com.vaadin</groupId>
<artifactId>vaadin-bom</artifactId>
<version>25.4-SNAPSHOT</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>Replace the application's own |
|
…precache-fallback # Conflicts: # flow-tests/test-pwa/pom-devbundle.xml
|
Closing as superseded by #24807, which already serves precached dev-bundle files from the precache instead of the network-first route, so this fallback is no longer needed. |
Review risk: two-way door — one fallback in the generated service worker; affects only requests that already fail on the network.
Behavior deviation · flow-server (service worker) · apps with
@PWArunning in development mode with the dev bundleBackground — service worker routes. Workbox serves a request with the first route that matches it. A route registered before
precacheAndRoutetakes precedence over the precache, and aNetworkFirststrategy only falls back to the runtime cache, which is filled by requests the service worker has already handled.In development mode with the dev bundle, reloading a
@PWAapp while offline after the first visit shows a blank page instead of the offline page. The bundle scripts are precached, but were never served from the precache. Opening the app a second time while online was the only workaround.Risks:
Context. The dev-mode
/VAADIN/*network-first route was added for the Vite dev server, whose files are not precached. The dev bundle's files are precached, so on the first offline reload every bundle script failed and the app shell never started, and the offline stub could not be shown.NetworkFirststrategy that returnsmatchPrecache(request)fromhandlerDidError, so a failed request falls back to the precache after the runtime cache.test-pwa/pom-devbundle.xml), following the existingtest-themes/pom-devbundle.xml, so the PWA tests also run without hotdeploy.Working on behalf of Artur