fix(service-worker): cache opaque responses in data groups with freshness strategy (#30977)

gkalpak · alxhub · commit d7be38f84b76 · 2019-06-27T09:52:29.000-07:00
Previously, (presummably due to a typo) the `okToCacheOpaque` argument of `DataGroup#cacheResponse()` was essentially never taken into account (since opaque responses have a non-200 status code and thus `res.ok` is always false). This commit fixes the typo, which allows opaque responses to be cached when `okToCacheOpaque` is true (i.e. in data groups using the `freshness` strategy). Fixes #30968 PR Close #30977
diff --git a/packages/service-worker/worker/src/data.ts b/packages/service-worker/worker/src/data.ts
@@ -481,10 +481,10 @@ export class DataGroup {
    * If the request times out on the server, an error will be returned but the real network
    * request will still be running in the background, to be cached when it completes.
    */
-  private async cacheResponse(
-      req: Request, res: Response, lru: LruList, okToCacheOpaque: boolean = false): Promise<void> {
+  private async cacheResponse(req: Request, res: Response, lru: LruList, okToCacheOpaque = false):
+      Promise<void> {
     // Only cache successful responses.
-    if (!res.ok || (okToCacheOpaque && res.type === 'opaque')) {
+    if (!(res.ok || (okToCacheOpaque && res.type === 'opaque'))) {
       return;
     }
 
diff --git a/packages/service-worker/worker/test/data_spec.ts b/packages/service-worker/worker/test/data_spec.ts
@@ -150,6 +150,14 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
         server.assertNoOtherRequests();
       });
 
+      it('does not cache opaque responses', async() => {
+        expect(await makeNoCorsRequest(scope, '/api/test')).toBe('');
+        server.assertSawRequestFor('/api/test');
+
+        expect(await makeNoCorsRequest(scope, '/api/test')).toBe('');
+        server.assertSawRequestFor('/api/test');
+      });
+
       it('refreshes after awhile', async() => {
         expect(await makeRequest(scope, '/api/test')).toEqual('version 1');
         server.clearRequests();
@@ -199,6 +207,16 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
         serverUpdate.assertNoOtherRequests();
       });
 
+      it('caches opaque responses', async() => {
+        expect(await makeNoCorsRequest(scope, '/fresh/data')).toBe('');
+        server.assertSawRequestFor('/fresh/data');
+
+        server.online = false;
+
+        expect(await makeRequest(scope, '/fresh/data')).toBe('');
+        server.assertNoOtherRequests();
+      });
+
       it('falls back on the cache when server times out', async() => {
         expect(await makeRequest(scope, '/fresh/data')).toEqual('this is fresh data');
         server.assertSawRequestFor('/fresh/data');
@@ -250,9 +268,18 @@ function makeRequest(scope: SwTestHarness, url: string, clientId?: string): Prom
   return done.then(() => resTextPromise);
 }
 
+function makeNoCorsRequest(
+    scope: SwTestHarness, url: string, clientId?: string): Promise<String|null> {
+  const req = new MockRequest(url, {mode: 'no-cors'});
+  const [resTextPromise, done] = makePendingRequest(scope, req, clientId);
+  return done.then(() => resTextPromise);
+}
+
 function makePendingRequest(
-    scope: SwTestHarness, url: string, clientId?: string): [Promise<string|null>, Promise<void>] {
-  const [resPromise, done] = scope.handleFetch(new MockRequest(url), clientId || 'default');
+    scope: SwTestHarness, urlOrReq: string | MockRequest,
+    clientId?: string): [Promise<string|null>, Promise<void>] {
+  const req = (typeof urlOrReq === 'string') ? new MockRequest(urlOrReq) : urlOrReq;
+  const [resPromise, done] = scope.handleFetch(req, clientId || 'default');
   return [
     resPromise.then<string|null>(res => res ? res.text() : null),
     done,
