From 52d43a99ef9384a970ae926aada80cc9788aca6d Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Fri, 25 May 2018 15:15:42 +0300 Subject: [PATCH] fix(service-worker): avoid network requests when looking up hashed resources in cache (#24127) PR Close #24127 --- .../service-worker/worker/src/app-version.ts | 12 +--- .../service-worker/worker/test/happy_spec.ts | 57 +++++++++++++++++-- 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/packages/service-worker/worker/src/app-version.ts b/packages/service-worker/worker/src/app-version.ts index e766dcc8e3..1329c9aaf4 100644 --- a/packages/service-worker/worker/src/app-version.ts +++ b/packages/service-worker/worker/src/app-version.ts @@ -198,8 +198,6 @@ export class AppVersion implements UpdateSource { * Check this version for a given resource with a particular hash. */ async lookupResourceWithHash(url: string, hash: string): Promise { - const req = this.adapter.newRequest(url); - // Verify that this version has the requested resource cached. If not, // there's no point in trying. if (!this.hashTable.has(url)) { @@ -208,16 +206,12 @@ export class AppVersion implements UpdateSource { // Next, check whether the resource has the correct hash. If not, any cached // response isn't usable. - if (this.hashTable.get(url) ! !== hash) { + if (this.hashTable.get(url) !== hash) { return null; } - // TODO: no-op context and appropriate contract. Currently this is a violation - // of the typings and could cause issues if handleFetch() has side effects. A - // better strategy to deal with side effects is needed. - // TODO: this could result in network fetches if the response is lazy. Refactor - // to avoid them. - return this.handleFetch(req, null !); + const cacheState = await this.lookupResourceWithoutHash(url); + return cacheState && cacheState.response; } /** diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index e444ca1502..1b558ef799 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -24,6 +24,9 @@ const dist = .addFile('/baz.txt', 'this is baz') .addFile('/qux.txt', 'this is qux') .addFile('/quux.txt', 'this is quux') + .addFile('/quuux.txt', 'this is quuux') + .addFile('/lazy/unchanged1.txt', 'this is unchanged (1)') + .addFile('/lazy/unchanged2.txt', 'this is unchanged (2)') .addUnhashedFile('/unhashed/a.txt', 'this is unhashed', {'Cache-Control': 'max-age=10'}) .build(); @@ -34,6 +37,9 @@ const distUpdate = .addFile('/baz.txt', 'this is baz v2') .addFile('/qux.txt', 'this is qux v2') .addFile('/quux.txt', 'this is quux v2') + .addFile('/quuux.txt', 'this is quuux v2') + .addFile('/lazy/unchanged1.txt', 'this is unchanged (1)') + .addFile('/lazy/unchanged2.txt', 'this is unchanged (2)') .addUnhashedFile('/unhashed/a.txt', 'this is unhashed v2', {'Cache-Control': 'max-age=10'}) .addUnhashedFile('/ignored/file1', 'this is not handled by the SW') .addUnhashedFile('/ignored/dir/file2', 'this is not handled by the SW either') @@ -92,7 +98,12 @@ const manifest: Manifest = { name: 'lazy_prefetch', installMode: 'lazy', updateMode: 'prefetch', - urls: ['/quux.txt'], + urls: [ + '/quux.txt', + '/quuux.txt', + '/lazy/unchanged1.txt', + '/lazy/unchanged2.txt', + ], patterns: [], } ], @@ -134,7 +145,12 @@ const manifestUpdate: Manifest = { name: 'lazy_prefetch', installMode: 'lazy', updateMode: 'prefetch', - urls: ['/quux.txt'], + urls: [ + '/quux.txt', + '/quuux.txt', + '/lazy/unchanged1.txt', + '/lazy/unchanged2.txt', + ], patterns: [], } ], @@ -528,14 +544,45 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate)); async_it('prefetches updates to lazy cache when set', async() => { expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo'); await driver.initialized; - expect(await makeRequest(scope, '/quux.txt')).toEqual('this is quux'); + // Fetch some files from the `lazy_prefetch` asset group. + expect(await makeRequest(scope, '/quux.txt')).toEqual('this is quux'); + expect(await makeRequest(scope, '/lazy/unchanged1.txt')).toEqual('this is unchanged (1)'); + + // Install update. scope.updateServerState(serverUpdate); - expect(await driver.checkForUpdate()).toEqual(true); + expect(await driver.checkForUpdate()).toBe(true); + + // Previously requested and changed: Fetch from network. serverUpdate.assertSawRequestFor('/quux.txt'); + // Never requested and changed: Don't fetch. + serverUpdate.assertNoRequestFor('/quuux.txt'); + // Previously requested and unchanged: Fetch from cache. + serverUpdate.assertNoRequestFor('/lazy/unchanged1.txt'); + // Never requested and unchanged: Don't fetch. + serverUpdate.assertNoRequestFor('/lazy/unchanged2.txt'); + serverUpdate.clearRequests(); + + // Update client. await driver.updateClient(await scope.clients.get('default')); - expect(await makeRequest(scope, '/quux.txt')).toEqual('this is quux v2'); + + // Already cached. + expect(await makeRequest(scope, '/quux.txt')).toBe('this is quux v2'); + serverUpdate.assertNoOtherRequests(); + + // Not cached: Fetch from network. + expect(await makeRequest(scope, '/quuux.txt')).toBe('this is quuux v2'); + serverUpdate.assertSawRequestFor('/quuux.txt'); + + // Already cached (copied from old cache). + expect(await makeRequest(scope, '/lazy/unchanged1.txt')).toBe('this is unchanged (1)'); + serverUpdate.assertNoOtherRequests(); + + // Not cached: Fetch from network. + expect(await makeRequest(scope, '/lazy/unchanged2.txt')).toBe('this is unchanged (2)'); + serverUpdate.assertSawRequestFor('/lazy/unchanged2.txt'); + serverUpdate.assertNoOtherRequests(); });