refactor(service-worker): minor refactorings to improve readability/maintainability (#42622)

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.

PR Close #42622

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

@@ -68,38 +68,34 @@ export class AppVersion implements UpdateSource {
 
   constructor(
       private scope: ServiceWorkerGlobalScope, private adapter: Adapter, private database: Database,
-      private idle: IdleScheduler, private debugHandler: DebugHandler, readonly manifest: Manifest,
+      idle: IdleScheduler, private debugHandler: DebugHandler, readonly manifest: Manifest,
       readonly manifestHash: string) {
     // The hashTable within the manifest is an Object - convert it to a Map for easier lookups.
-    Object.keys(this.manifest.hashTable).forEach(url => {
+    Object.keys(manifest.hashTable).forEach(url => {
-      this.hashTable.set(adapter.normalizeUrl(url), this.manifest.hashTable[url]);
+      this.hashTable.set(adapter.normalizeUrl(url), manifest.hashTable[url]);
     });
 
     // Process each `AssetGroup` declared in the manifest. Each declared group gets an `AssetGroup`
-    // instance
+    // instance created for it, of a type that depends on the configuration mode.
-    // created for it, of a type that depends on the configuration mode.
+    const assetCacheNamePrefix = `${adapter.cacheNamePrefix}:${manifestHash}:assets`;
     this.assetGroups = (manifest.assetGroups || []).map(config => {
-      // Every asset group has a cache that's prefixed by the manifest hash and the name of the
-      // group.
-      const prefix = `${adapter.cacheNamePrefix}:${this.manifestHash}:assets`;
       // Check the caching mode, which determines when resources will be fetched/updated.
       switch (config.installMode) {
         case 'prefetch':
           return new PrefetchAssetGroup(
-              this.scope, this.adapter, this.idle, config, this.hashTable, this.database, prefix);
+              scope, adapter, idle, config, this.hashTable, database, assetCacheNamePrefix);
         case 'lazy':
           return new LazyAssetGroup(
-              this.scope, this.adapter, this.idle, config, this.hashTable, this.database, prefix);
+              scope, adapter, idle, config, this.hashTable, database, assetCacheNamePrefix);
       }
     });
 
     // Process each `DataGroup` declared in the manifest.
-    this.dataGroups =
+    this.dataGroups = (manifest.dataGroups || [])
-        (manifest.dataGroups || [])
+                          .map(
-            .map(
+                              config => new DataGroup(
-                config => new DataGroup(
+                                  scope, adapter, config, database, debugHandler,
-                    this.scope, this.adapter, config, this.database, this.debugHandler,
+                                  `${adapter.cacheNamePrefix}:${config.version}:data`));
-                    `${adapter.cacheNamePrefix}:${config.version}:data`));
 
     // This keeps backwards compatibility with app versions without navigation urls.
     // Fix: https://github.com/angular/angular/issues/27209

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

@@ -38,7 +38,7 @@ export abstract class AssetGroup {
 
   /**
    * A Promise which resolves to the `Cache` used to back this asset group. This
-   * is openedfrom the constructor.
+   * is opened from the constructor.
    */
   protected cache: Promise<Cache>;
 
@@ -55,7 +55,8 @@ export abstract class AssetGroup {
   constructor(
       protected scope: ServiceWorkerGlobalScope, protected adapter: Adapter,
       protected idle: IdleScheduler, protected config: AssetGroupConfig,
-      protected hashes: Map<string, string>, protected db: Database, protected prefix: string) {
+      protected hashes: Map<string, string>, protected db: Database,
+      protected cacheNamePrefix: string) {
     this.name = config.name;
 
     // Normalize the config's URLs to take the ServiceWorker's scope into account.
@@ -65,13 +66,13 @@ export abstract class AssetGroup {
     this.patterns = config.patterns.map(pattern => new RegExp(pattern));
 
     // This is the primary cache, which holds all of the cached requests for this group. If a
-    // resource
+    // resource isn't in this cache, it hasn't been fetched yet.
-    // isn't in this cache, it hasn't been fetched yet.
+    this.cache = scope.caches.open(`${cacheNamePrefix}:${config.name}:cache`);
-    this.cache = scope.caches.open(`${this.prefix}:${config.name}:cache`);
 
     // This is the metadata table, which holds specific information for each cached URL, such as
     // the timestamp of when it was added to the cache.
-    this.metadata = this.db.open(`${this.prefix}:${config.name}:meta`, config.cacheQueryOptions);
+    this.metadata =
+        this.db.open(`${cacheNamePrefix}:${config.name}:meta`, config.cacheQueryOptions);
   }
 
   async cacheStatus(url: string): Promise<UpdateCacheStatus> {
@@ -102,8 +103,8 @@ export abstract class AssetGroup {
    * Clean up all the cached data for this group.
    */
   async cleanup(): Promise<void> {
-    await this.scope.caches.delete(`${this.prefix}:${this.config.name}:cache`);
+    await this.scope.caches.delete(`${this.cacheNamePrefix}:${this.config.name}:cache`);
-    await this.db.delete(`${this.prefix}:${this.config.name}:meta`);
+    await this.db.delete(`${this.cacheNamePrefix}:${this.config.name}:meta`);
   }
 
   /**
@@ -136,7 +137,8 @@ export abstract class AssetGroup {
           // to make sure it's still usable.
           if (await this.needToRevalidate(req, cachedResponse)) {
             this.idle.schedule(
-                `revalidate(${this.prefix}, ${this.config.name}): ${req.url}`, async () => {
+                `revalidate(${this.cacheNamePrefix}, ${this.config.name}): ${req.url}`,
+                async () => {
                   await this.fetchAndCacheOnce(req);
                 });
           }
@@ -314,7 +316,7 @@ export abstract class AssetGroup {
       try {
         // This response is safe to cache (as long as it's cloned). Wait until the cache operation
         // is complete.
-        const cache = await this.scope.caches.open(`${this.prefix}:${this.config.name}:cache`);
+        const cache = await this.cache;
         await cache.put(req, res.clone());
 
         // If the request is not hashed, update its metadata, especially the timestamp. This is

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

@@ -247,13 +247,13 @@ export class DataGroup {
   constructor(
       private scope: ServiceWorkerGlobalScope, private adapter: Adapter,
       private config: DataGroupConfig, private db: Database, private debugHandler: DebugHandler,
-      private prefix: string) {
+      private cacheNamePrefix: string) {
-    this.patterns = this.config.patterns.map(pattern => new RegExp(pattern));
+    this.patterns = config.patterns.map(pattern => new RegExp(pattern));
-    this.cache = this.scope.caches.open(`${this.prefix}:dynamic:${this.config.name}:cache`);
+    this.cache = scope.caches.open(`${cacheNamePrefix}:dynamic:${config.name}:cache`);
-    this.lruTable = this.db.open(
+    this.lruTable =
-        `${this.prefix}:dynamic:${this.config.name}:lru`, this.config.cacheQueryOptions);
+        this.db.open(`${cacheNamePrefix}:dynamic:${config.name}:lru`, config.cacheQueryOptions);
-    this.ageTable = this.db.open(
+    this.ageTable =
-        `${this.prefix}:dynamic:${this.config.name}:age`, this.config.cacheQueryOptions);
+        this.db.open(`${cacheNamePrefix}:dynamic:${config.name}:age`, config.cacheQueryOptions);
   }
 
   /**
@@ -550,9 +550,9 @@ export class DataGroup {
   async cleanup(): Promise<void> {
     // Remove both the cache and the database entries which track LRU stats.
     await Promise.all([
-      this.scope.caches.delete(`${this.prefix}:dynamic:${this.config.name}:cache`),
+      this.scope.caches.delete(`${this.cacheNamePrefix}:dynamic:${this.config.name}:cache`),
-      this.db.delete(`${this.prefix}:dynamic:${this.config.name}:age`),
+      this.db.delete(`${this.cacheNamePrefix}:dynamic:${this.config.name}:age`),
-      this.db.delete(`${this.prefix}:dynamic:${this.config.name}:lru`),
+      this.db.delete(`${this.cacheNamePrefix}:dynamic:${this.config.name}:lru`),
     ]);
   }
 

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

@@ -15,7 +15,8 @@ import {Database, NotFound, Table} from './database';
  * state within mock `Response` objects.
  */
 export class CacheDatabase implements Database {
-  private tables = new Map<string, Promise<CacheTable>>();
+  private cacheNamePrefix = `${this.adapter.cacheNamePrefix}:db`;
+  private tables = new Map<string, CacheTable>();
 
   constructor(private scope: ServiceWorkerGlobalScope, private adapter: Adapter) {}
 
@@ -23,19 +24,20 @@ export class CacheDatabase implements Database {
     if (this.tables.has(name)) {
       this.tables.delete(name);
     }
-    return this.scope.caches.delete(`${this.adapter.cacheNamePrefix}:db:${name}`);
+    return this.scope.caches.delete(`${this.cacheNamePrefix}:${name}`);
   }
 
-  list(): Promise<string[]> {
+  async list(): Promise<string[]> {
-    return this.scope.caches.keys().then(
+    const prefix = `${this.cacheNamePrefix}:`;
-        keys => keys.filter(key => key.startsWith(`${this.adapter.cacheNamePrefix}:db:`)));
+    const allCacheNames = await this.scope.caches.keys();
+    const dbCacheNames = allCacheNames.filter(name => name.startsWith(prefix));
+    return dbCacheNames;
   }
 
-  open(name: string, cacheQueryOptions?: CacheQueryOptions): Promise<Table> {
+  async open(name: string, cacheQueryOptions?: CacheQueryOptions): Promise<Table> {
     if (!this.tables.has(name)) {
-      const table =
+      const cache = await this.scope.caches.open(`${this.cacheNamePrefix}:${name}`);
-          this.scope.caches.open(`${this.adapter.cacheNamePrefix}:db:${name}`)
+      const table = new CacheTable(name, cache, this.adapter, cacheQueryOptions);
-              .then(cache => new CacheTable(name, cache, this.adapter, cacheQueryOptions));
       this.tables.set(name, table);
     }
     return this.tables.get(name)!;

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

@@ -556,8 +556,7 @@ export class Driver implements Debuggable, UpdateSource {
       // server and building up an empty initial state.
       const manifest = await this.fetchLatestManifest();
       const hash = hashManifest(manifest);
-      manifests = {};
+      manifests = {[hash]: manifest};
-      manifests[hash] = manifest;
       assignments = {};
       latest = {latest: hash};
 

packages/service-worker/worker/testing/scope.ts

@@ -108,7 +108,7 @@ export class MockClients implements Clients {
 export class SwTestHarness extends Adapter implements ServiceWorkerGlobalScope, Context {
   readonly clients = new MockClients();
   private eventHandlers = new Map<string, Function>();
-  private skippedWaiting = true;
+  private skippedWaiting = false;
 
   private selfMessageQueue: any[] = [];
   autoAdvanceTime = false;