fix(service-worker): do not enter degraded mode when offline (#22883)

gkalpak · alxhub · commit ae9c25ff3dd6 · 2018-03-28T10:02:18.000-07:00
Previously, when trying to fetch `ngsw.json` (e.g. during `checkForUpdate()`) while either the client or the server were offline, the ServiceWorker would enter a degrade mode, where only existing clients would be served. This essentially meant that the ServiceWorker didn't work offline. This commit fixes it by differentiating offline errors and not entering degraded mode. The ServiceWorker will remain in the current mode until connectivity to the server is restored. Fixes #21636 PR Close #22883
diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts
@@ -607,16 +607,22 @@ export class Driver implements Debuggable, UpdateSource {
 
   /**
    * Retrieve a copy of the latest manifest from the server.
+   * Return `null` if `ignoreOfflineError` is true (default: false) and the server or client are
+   * offline (detected as response status 504).
    */
-  private async fetchLatestManifest(): Promise<Manifest> {
+  private async fetchLatestManifest(ignoreOfflineError?: false): Promise<Manifest>;
+  private async fetchLatestManifest(ignoreOfflineError: true): Promise<Manifest | null>;
+  private async fetchLatestManifest(ignoreOfflineError = false): Promise<Manifest | null> {
     const res =
         await this.safeFetch(this.adapter.newRequest('ngsw.json?ngsw-cache-bust=' + Math.random()));
     if (!res.ok) {
       if (res.status === 404) {
         await this.deleteAllCaches();
         await this.scope.registration.unregister();
+      } else if (res.status === 504 && ignoreOfflineError) {
+        return null;
       }
-      throw new Error('Manifest fetch failed!');
+      throw new Error(`Manifest fetch failed! (status: ${res.status})`);
     }
     this.lastUpdateCheck = this.adapter.time;
     return res.json();
@@ -728,7 +734,15 @@ export class Driver implements Debuggable, UpdateSource {
   async checkForUpdate(): Promise<boolean> {
     let hash: string = '(unknown)';
     try {
-      const manifest = await this.fetchLatestManifest();
+      const manifest = await this.fetchLatestManifest(true);
+
+      if (manifest === null) {
+        // Client or server offline. Unable to check for updates at this time.
+        // Continue to service clients (existing and new).
+        this.debugger.log('Check for update aborted. (Client or server offline.)');
+        return false;
+      }
+
       hash = hashManifest(manifest);
 
       // Check whether this is really an update.
@@ -972,4 +986,4 @@ function errorToString(error: any): string {
   } else {
     return `${error}`;
   }
-}
+}
diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts
@@ -516,6 +516,17 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
       expect(await scope.caches.keys()).toEqual([]);
     });
 
+    async_it('does not unregister or change state when offline (i.e. manifest 504s)', async() => {
+      expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
+      await driver.initialized;
+      server.online = false;
+
+      expect(await driver.checkForUpdate()).toEqual(false);
+      expect(driver.state).toEqual(DriverReadyState.NORMAL);
+      expect(scope.unregistered).toBeFalsy();
+      expect(await scope.caches.keys()).not.toEqual([]);
+    });
+
     describe('unhashed requests', () => {
       async_beforeEach(async() => {
         expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
diff --git a/packages/service-worker/worker/testing/mock.ts b/packages/service-worker/worker/testing/mock.ts
@@ -96,6 +96,7 @@ export class MockServerState {
   private gate: Promise<void> = Promise.resolve();
   private resolve: Function|null = null;
   private resolveNextRequest: Function;
+  online = true;
   nextRequest: Promise<Request>;
 
   constructor(private resources: Map<string, Response>, private errors: Set<string>) {
@@ -108,6 +109,10 @@ export class MockServerState {
 
     await this.gate;
 
+    if (!this.online) {
+      throw new Error('Offline.');
+    }
+
     if (req.credentials === 'include') {
       return new MockResponse(null, {status: 0, statusText: '', type: 'opaque'});
     }
@@ -171,6 +176,7 @@ export class MockServerState {
     this.nextRequest = new Promise(resolve => { this.resolveNextRequest = resolve; });
     this.gate = Promise.resolve();
     this.resolve = null;
+    this.online = true;
   }
 }
 
