diff --git a/packages/service-worker/worker/src/assets.ts b/packages/service-worker/worker/src/assets.ts index c2f4222bdd..ebaeba4fd2 100644 --- a/packages/service-worker/worker/src/assets.ts +++ b/packages/service-worker/worker/src/assets.ts @@ -388,18 +388,17 @@ export abstract class AssetGroup { // a stale response. // Fetch the resource from the network (possibly hitting the HTTP cache). - const networkResult = await this.safeFetch(req); + let response = await this.safeFetch(req); - // Decide whether a cache-busted request is necessary. It might be for two independent - // reasons: either the non-cache-busted request failed (hopefully transiently) or if the - // hash of the content retrieved does not match the canonical hash from the manifest. It's - // only valid to access the content of the first response if the request was successful. - let makeCacheBustedRequest: boolean = !networkResult.ok; - if (networkResult.ok) { + // Decide whether a cache-busted request is necessary. A cache-busted request is necessary + // only if the request was successful but the hash of the retrieved contents does not match + // the canonical hash from the manifest. + let makeCacheBustedRequest = response.ok; + if (makeCacheBustedRequest) { // The request was successful. A cache-busted request is only necessary if the hashes - // don't match. Compare them, making sure to clone the response so it can be used later - // if it proves to be valid. - const fetchedHash = sha1Binary(await networkResult.clone().arrayBuffer()); + // don't match. + // (Make sure to clone the response so it can be used later if it proves to be valid.) + const fetchedHash = sha1Binary(await response.clone().arrayBuffer()); makeCacheBustedRequest = (fetchedHash !== canonicalHash); } @@ -411,39 +410,34 @@ export abstract class AssetGroup { // request will differentiate these two situations. // TODO: handle case where the URL has parameters already (unlikely for assets). const cacheBustReq = this.adapter.newRequest(this.cacheBust(req.url)); - const cacheBustedResult = await this.safeFetch(cacheBustReq); + response = await this.safeFetch(cacheBustReq); - // If the response was unsuccessful, there's nothing more that can be done. - if (!cacheBustedResult.ok) { - if (cacheBustedResult.status === 404) { - throw new SwUnrecoverableStateError( - `Failed to retrieve hashed resource from the server. (AssetGroup: ${ - this.config.name} | URL: ${url})`); - } else { - throw new SwCriticalError( - `Response not Ok (cacheBustedFetchFromNetwork): cache busted request for ${ - req.url} returned response ${cacheBustedResult.status} ${ - cacheBustedResult.statusText}`); + // If the response was successful, check the contents against the canonical hash. + if (response.ok) { + // Hash the contents. + // (Make sure to clone the response so it can be used later if it proves to be valid.) + const cacheBustedHash = sha1Binary(await response.clone().arrayBuffer()); + + // If the cache-busted version doesn't match, then the manifest is not an accurate + // representation of the server's current set of files, and the SW should give up. + if (canonicalHash !== cacheBustedHash) { + throw new SwCriticalError(`Hash mismatch (cacheBustedFetchFromNetwork): ${ + req.url}: expected ${canonicalHash}, got ${cacheBustedHash} (after cache busting)`); } } - - // Hash the contents. - const cacheBustedHash = sha1Binary(await cacheBustedResult.clone().arrayBuffer()); - - // If the cache-busted version doesn't match, then the manifest is not an accurate - // representation of the server's current set of files, and the SW should give up. - if (canonicalHash !== cacheBustedHash) { - throw new SwCriticalError(`Hash mismatch (cacheBustedFetchFromNetwork): ${ - req.url}: expected ${canonicalHash}, got ${cacheBustedHash} (after cache busting)`); - } - - // If it does match, then use the cache-busted result. - return cacheBustedResult; } - // Excellent, the version from the network matched on the first try, with no need for - // cache-busting. Use it. - return networkResult; + // At this point, `response` is either successful with a matching hash or is unsuccessful. + // Before returning it, check whether it failed with a 404 status. This would signify an + // unrecoverable state. + if (!response.ok && (response.status === 404)) { + throw new SwUnrecoverableStateError( + `Failed to retrieve hashed resource from the server. (AssetGroup: ${ + this.config.name} | URL: ${url})`); + } + + // Return the response (successful or unsuccessful). + return response; } else { // This URL doesn't exist in our hash database, so it must be requested directly. return this.safeFetch(req); diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index 09c5a374d7..31c72b607a 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -794,7 +794,7 @@ describe('Driver', () => { serverUpdate.assertNoOtherRequests(); }); - it('should bypass serviceworker on ngsw-bypass parameter', async () => { + it('bypasses the ServiceWorker on `ngsw-bypass` parameter', async () => { // NOTE: // Requests that bypass the SW are not handled at all in the mock implementation of `scope`, // therefore no requests reach the server. @@ -1115,7 +1115,7 @@ describe('Driver', () => { server.assertNoOtherRequests(); }); - it(`doesn't error when 'Cache-Control' is 'no-cache'`, async () => { + it(`don't error when 'Cache-Control' is 'no-cache'`, async () => { expect(await makeRequest(scope, '/unhashed/b.txt')).toEqual('this is unhashed b'); server.assertSawRequestFor('/unhashed/b.txt'); expect(await makeRequest(scope, '/unhashed/b.txt')).toEqual('this is unhashed b'); @@ -1744,6 +1744,25 @@ describe('Driver', () => { expect(requestUrls2).toContain(httpsRequestUrl); }); + it('does not enter degraded mode when offline while fetching an uncached asset', async () => { + // Trigger SW initialization and wait for it to complete. + expect(await makeRequest(scope, '/foo.txt')).toBe('this is foo'); + await driver.initialized; + + // Request an uncached asset while offline. + // The SW will not be able to get the content, but it should not enter a degraded mode either. + server.online = false; + await expectAsync(makeRequest(scope, '/baz.txt')) + .toBeRejectedWithError( + 'Response not Ok (fetchAndCacheOnce): request for /baz.txt returned response 504 Gateway Timeout'); + expect(driver.state).toBe(DriverReadyState.NORMAL); + + // Once we are back online, everything should work as expected. + server.online = true; + expect(await makeRequest(scope, '/baz.txt')).toBe('this is baz'); + expect(driver.state).toBe(DriverReadyState.NORMAL); + }); + describe('unrecoverable state', () => { const generateMockServerState = (fileSystem: MockFileSystem) => { const manifest: Manifest = {