fix(service-worker): improvements/fixes regarding ServiceWorker caches #42622

gkalpak · 2021-06-22T16:45:04Z

Several improvements and fixes regarding the ServiceWorker caches (naming, accessing, clean-up).
See individual commits for details.

Fixes #41728.

NOTE: Theoretically, this could land on patch as well. But since it changes some of the SW caches names, let's be conservative and land in a minor release.

mary-poppins · 2021-06-22T17:00:19Z

You can preview 5eb1a5b at https://pr42622-5eb1a5b.ngbuilds.io/.

mary-poppins · 2021-06-22T17:12:54Z

You can preview 2159d08 at https://pr42622-2159d08.ngbuilds.io/.

AndrewKushnir

LGTM overall, I just left a few comments. The most important one is related to a possible typo in the NamedCacheStorage.has code.

packages/service-worker/worker/src/assets.ts

packages/service-worker/worker/src/debug.ts

packages/service-worker/worker/src/db-cache.ts

packages/service-worker/worker/src/data.ts

packages/service-worker/worker/main.ts

packages/service-worker/worker/src/service-worker.d.ts

packages/service-worker/worker/src/named-cache-storage.ts

packages/service-worker/worker/src/app-version.ts

AndrewKushnir · 2021-06-22T19:12:44Z

@gkalpak FYI I was going through the commits one-by-one, so some of the comments are marked as "outdated" in GitHub UI, feel free to ignore them if they are not relevant and/or already resolved.

gkalpak · 2021-06-23T12:36:59Z

Thx for the review, @AndrewKushnir ✨
I have addressed most of the comments (in fixup commits) and replied to the rest. I have left a couple of review comments unresolved to get your input. PTAL

mary-poppins · 2021-06-23T12:46:10Z

You can preview d0ddf1b at https://pr42622-d0ddf1b.ngbuilds.io/.

AndrewKushnir

👍

ngbot · 2021-06-23T15:55:23Z

I see that you just added the action: merge label, but the following checks are still failing:
     status "ci/angular: size" is failing
     status "ci/circleci: build-ivy-npm-packages" is failing
     status "ci/circleci: build-npm-packages" is failing
     status "ci/circleci: legacy-unit-tests-saucelabs" is failing
     status "ci/circleci: test" is failing
     status "ci/circleci: test_ivy_aot" is failing
     status "ci/circleci: test_zonejs" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

…aintainability This commit makes some minor refactorings to improve the code readability and maintainability, including: - Avoiding code duplication. - Using more descriptive variable names. - Using `async/await` instead of `Promise#then()`. - Accessing variables directly instead of via `this` when possible.

This commit includes the ServiceWorker version in the debug info shown at `/ngsw/state` to make it easier to know what version of the ServiceWorker script is controlling the page.

…ables `CacheDatabase` uses the un-prefixed table names to interact with database tables. However, the `list()` method returns the raw, prefixed table names (which are not useful, since they cannot be used to open/delete a table). This commit fixes this by removing the prefix from the cache names returned by the `list()` method. NOTE: This method is currently not used anywhere, so this change does not affect the ServiceWorker behavior.

…base` tables

This commit improves the cache names generated by the ServiceWorker by making them shorter and non-repetitive. In particular, the following changes are made: - Data-group cache names no longer include the `dynamic` infix, since it does not add any value. Before: `ngsw:<...>:data:dynamic:<...>` After: `ngsw:<...>:data:<...>` - `CacheDatabase` table names no longer include the `ngsw:<path>` prefix twice. Before: `ngsw:<path>:db:ngsw:<path>:<...>` After: `ngsw:<path>:db:<...>` NOTE 1: This change will result in different cache names being generated for the same app-versions with the new SericeWorker script. This means that some of the previously cached data will need to be re-downloaded (because the ServiceWorker will not be able to re-use the old caches), but that should be transparent for the end user. While possible, adding logic to allow the ServiceWorker to retrieve data from the old caches is not worth the extra complecity and maintenance cost. NOTE 2: Generating different cache names for some of the caches means that the ServiceWorker will not be able to clean-up some of the old caches. This will be taken care of in a subsequent commit that will rework the clean-up logic to be more robust (covering changes such as this one and other edgecases).

…t the ServiceWorker This commit simplifies/systemizes accessing the `CacheStorage` through a wrapper, with the following benefits: - Ensuring a consistent cache name prefix is used for all caches (without having to repeat the prefix in different places). - Allowing referring to caches using their name without the common cache name prefix. - Exposing the cache name on cache instances, which for example makes it easier to delete caches without having to keep track of the name used to create them.

…roughout the ServiceWorker

…d state Previously, obsolete caches were only cleaned up when successfully loading the stored state. When the state failed to be loaded, cleaning up the caches would be skipped until the next SW initialization. This commit changes this, ensuring that the caches are cleaned up regardless if the stored state was loaded successfully or not.

Previously, the SW was only able to clean up caches for app-versions found in the `Driver`'s `versions` map. If (for some reason) the `Driver` failed to load a valid stored state (including app-versions) and ended up with an [empty `versions` map][1], any obsolete versions would remain in the cache storage. This case was rare but possible. This commit makes the cache clean-up logic more robust by ensuring that all app-version caches are removed unless they are currently used by the SW to serve active clients (with the exception of the latest app-version, which is always retained). Fixes angular#41728 [1]: https://github.com/angular/angular/blob/9de65dbdceac3077881fbc49717f33d0f379e21d/packages/service-worker/worker/src/driver.ts#L515-L529

mary-poppins · 2021-06-24T15:56:06Z

You can preview 612ba53 at https://pr42622-612ba53.ngbuilds.io/.

…2622) This commit includes the ServiceWorker version in the debug info shown at `/ngsw/state` to make it easier to know what version of the ServiceWorker script is controlling the page. PR Close #42622

…ables (#42622) `CacheDatabase` uses the un-prefixed table names to interact with database tables. However, the `list()` method returns the raw, prefixed table names (which are not useful, since they cannot be used to open/delete a table). This commit fixes this by removing the prefix from the cache names returned by the `list()` method. NOTE: This method is currently not used anywhere, so this change does not affect the ServiceWorker behavior. PR Close #42622

This commit improves the cache names generated by the ServiceWorker by making them shorter and non-repetitive. In particular, the following changes are made: - Data-group cache names no longer include the `dynamic` infix, since it does not add any value. Before: `ngsw:<...>:data:dynamic:<...>` After: `ngsw:<...>:data:<...>` - `CacheDatabase` table names no longer include the `ngsw:<path>` prefix twice. Before: `ngsw:<path>:db:ngsw:<path>:<...>` After: `ngsw:<path>:db:<...>` NOTE 1: This change will result in different cache names being generated for the same app-versions with the new SericeWorker script. This means that some of the previously cached data will need to be re-downloaded (because the ServiceWorker will not be able to re-use the old caches), but that should be transparent for the end user. While possible, adding logic to allow the ServiceWorker to retrieve data from the old caches is not worth the extra complecity and maintenance cost. NOTE 2: Generating different cache names for some of the caches means that the ServiceWorker will not be able to clean-up some of the old caches. This will be taken care of in a subsequent commit that will rework the clean-up logic to be more robust (covering changes such as this one and other edgecases). PR Close #42622

…t the ServiceWorker (#42622) This commit simplifies/systemizes accessing the `CacheStorage` through a wrapper, with the following benefits: - Ensuring a consistent cache name prefix is used for all caches (without having to repeat the prefix in different places). - Allowing referring to caches using their name without the common cache name prefix. - Exposing the cache name on cache instances, which for example makes it easier to delete caches without having to keep track of the name used to create them. PR Close #42622

…d state (#42622) Previously, obsolete caches were only cleaned up when successfully loading the stored state. When the state failed to be loaded, cleaning up the caches would be skipped until the next SW initialization. This commit changes this, ensuring that the caches are cleaned up regardless if the stored state was loaded successfully or not. PR Close #42622

…2622) Previously, the SW was only able to clean up caches for app-versions found in the `Driver`'s `versions` map. If (for some reason) the `Driver` failed to load a valid stored state (including app-versions) and ended up with an [empty `versions` map][1], any obsolete versions would remain in the cache storage. This case was rare but possible. This commit makes the cache clean-up logic more robust by ensuring that all app-version caches are removed unless they are currently used by the SW to serve active clients (with the exception of the latest app-version, which is always retained). Fixes #41728 [1]: https://github.com/angular/angular/blob/9de65dbdceac3077881fbc49717f33d0f379e21d/packages/service-worker/worker/src/driver.ts#L515-L529 PR Close #42622

gkalpak · 2021-07-05T12:45:12Z

This should also have fixed #20785.

Update `@angular/*` packages to latest 12.1.x versions (mainly to take advantage of recent ServiceWorker improvements, such as angular#42607 and angular#42622).

Update `@angular/*` packages to latest 12.1.x versions (mainly to take advantage of recent ServiceWorker improvements, such as #42607 and #42622). PR Close #42776

angular-automatic-lock-bot · 2021-08-05T16:32:13Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

gkalpak added type: bug/fix area: service-worker Issues related to the @angular/service-worker package target: minor This PR is targeted for the next minor release labels Jun 22, 2021

ngbot bot modified the milestone: Backlog Jun 22, 2021

google-cla bot added the cla: yes label Jun 22, 2021

gkalpak force-pushed the fix-sw-cache-cleanup branch from 818b56d to 5eb1a5b Compare June 22, 2021 16:51

gkalpak mentioned this pull request Jun 22, 2021

@angular/pwa doesn't clean up previous service worker version's cached content #41728

Closed

gkalpak force-pushed the fix-sw-cache-cleanup branch from 5eb1a5b to 2159d08 Compare June 22, 2021 17:02

gkalpak marked this pull request as ready for review June 22, 2021 17:32

pullapprove bot requested a review from AndrewKushnir June 22, 2021 17:33

gkalpak added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 22, 2021

AndrewKushnir approved these changes Jun 22, 2021

View reviewed changes

AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 22, 2021

gkalpak force-pushed the fix-sw-cache-cleanup branch 3 times, most recently from 3631031 to d0ddf1b Compare June 23, 2021 12:34

gkalpak requested a review from AndrewKushnir June 23, 2021 12:37

AndrewKushnir approved these changes Jun 23, 2021

View reviewed changes

gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 23, 2021

gkalpak added 10 commits June 24, 2021 08:45

feat(service-worker): include ServiceWorker version in debug info

0a3581b

This commit includes the ServiceWorker version in the debug info shown at `/ngsw/state` to make it easier to know what version of the ServiceWorker script is controlling the page.

fixup! fix(service-worker): use correct names when listing `CacheData…

3a8f195

…base` tables

fixup! refactor(service-worker): simplify accessing CacheStorage th…

3d38d0f

…roughout the ServiceWorker

fixup! fix(service-worker): ensure obsolete caches are always cleaned up

612ba53

jessicajaniuk force-pushed the fix-sw-cache-cleanup branch from d0ddf1b to 612ba53 Compare June 24, 2021 15:45

jessicajaniuk closed this in 4962ef5 Jun 24, 2021

gkalpak deleted the fix-sw-cache-cleanup branch June 24, 2021 17:46

gkalpak mentioned this pull request Jul 5, 2021

Datagroups defined in service worker config are purged when app is updated #20785

Closed

gkalpak mentioned this pull request Jul 5, 2021

build(docs-infra): update @angular/* to 12.1.1 #42776

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 5, 2021

fix(service-worker): improvements/fixes regarding ServiceWorker caches #42622

fix(service-worker): improvements/fixes regarding ServiceWorker caches #42622

Uh oh!

Conversation

gkalpak commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mary-poppins commented Jun 22, 2021

Uh oh!

mary-poppins commented Jun 22, 2021

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented Jun 22, 2021

Uh oh!

gkalpak commented Jun 23, 2021

Uh oh!

mary-poppins commented Jun 23, 2021

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

ngbot bot commented Jun 23, 2021

Uh oh!

mary-poppins commented Jun 24, 2021

Uh oh!

gkalpak commented Jul 5, 2021

Uh oh!

angular-automatic-lock-bot bot commented Aug 5, 2021

Uh oh!

Uh oh!

gkalpak commented Jun 22, 2021 •

edited

Loading