Precaching temp #1342
Precaching temp #1342
Conversation
|
I ended up changing the The reason for this is that a set of the precaching tests rely on being run after the activation logic has finished, which isn't true if we are waiting on the controllerchange event instead. |
infra/testing/activate-sw.js
Outdated
| } | ||
|
|
||
| navigator.serviceWorker.register(swUrl) |
There was a problem hiding this comment.
The background on why we were waiting for the correct SW to take control, rather than waiting on activation, is in w3c/ServiceWorker#799 (comment)
We've seen flakiness in unit tests in the past due to the small window of time in between when a service worker activates and when it takes control of a window client. If the window client makes a network request in that window of time, the correct service worker won't intercept it.
There was a problem hiding this comment.
So I saw flakiness the other way, so how about we check for both?
There was a problem hiding this comment.
Can we dig into the flakiness you saw in the other direction? Potentially outside of this PR, if you can restore the previous logic and supplement it with the additional logic that's now necessarily.
FWIW, a scenario in which a SW can take control of a page before it's been activated sounds like a (potentially serious) browser bug.
There was a problem hiding this comment.
Spoke to @jakearchibald about this as I don't think it is a bug.
Having multiple evens, one claiming and others doing work means:
All clients will be claimed straight away, but the service worker won't activate until promises passed to waitUntil resolve
This makes sense to me but does bring in to question whether we should do some other magic around clientsClaim to account for this.
There was a problem hiding this comment.
Wait, really? My understanding was that the service worker couldn't take control of a client until after it activated. Huh.
| const requests = await tempCache.keys(); | ||
| await Promise.all(requests.map(async (request) => { | ||
| const response = await tempCache.match(request); | ||
| await cacheWrapper.put(this._cacheName, request, response); |
There was a problem hiding this comment.
We have #1312 open to track the larger question of how to deal with quota issues that come up during precaching.
In the meantime, this PR introduces some overhead due to every response in the temp cache being duplicated first, and with all the temp entries only getting deleted after all of those duplicates are written.
What if you added in
await cacheWrapper.delete(this._getTempCacheName(), request);inside this loop, to clear out the temporary copy of each individual response after it's been duplicated to the final cache?
You can still leave the
await caches.delete(this._getTempCacheName());outside the loop, to delete the cache itself, though at that point the cache should have 0 entries in it.
|
@jeffposnick this has the changes you mentioned (good thinking on the clearing the temp cache as we go). There isn't currently a cacheWrapper delete function and not sure we should add one this late in the game for v3. Also changed the activate logic to wait for activate to finish and then check the current controller. |
PR-Bot Size PluginChanged File Sizes
New FilesNo new files have been added. All File SizesView Table
Workbox Aggregate Size Plugin☠️ WARNING ☠️We are using 158% of our max size budget. Total Size: 23.19KB Gzipped: 9.22KB |
|
LGTM. |
Fixes #1316
Precaching first occurs into a cache with
-tempappended to the end and during the activate step I copy the requests over to the originally intended cache.For the most part this seems like a minor and safe change. I extended the test that installs and activates two sets of assets to ensure new expectations are being met, there was a bug in the cache API where keys weren't updated and the integration test had a weird issue with Firefox where multiple
activateevents seem to enter into a race condition and we can't rely on the cache cleanup being done by the time the client is claimed.Let me know what you thinkg