Fix NetworkFirst waitUntil (#2744)

* Fix NetworkFirst waitUntil

NetworkFirst's design uses two promises (one for the network request, and one for the timeout). If the network request succeeds, then the timeout promise's timer is canceled. However, it attempted to wait until both promises were done before marking the event as done; this meant that, if the network request succeeded, then it never resolved its handlerDone / awaitComplete promise.

This fixes #2721.

* Add a test to cover the timeout-not-exceeded case

* Explicitly verify that the promise resolved before timeout

* Properly handle network fallback

Improve typings for waitUntil; since the revised logic uses a callback, it can't rely on type inference as much.

* Code review feedback

Follow coding style; remove eventDoneWaiting calls, since that duplicates donePromise.

packages/workbox-strategies/src/NetworkFirst.ts

@@ -100,7 +100,7 @@ class NetworkFirst extends Strategy {
       });
     }
 
-    const promises = [];
+    const promises: Promise<Response | undefined>[] = [];
     let timeoutId: number | undefined;
 
     if (this._networkTimeoutSeconds) {
@@ -113,20 +113,17 @@ class NetworkFirst extends Strategy {
         this._getNetworkPromise({timeoutId, request, logs, handler});
 
     promises.push(networkPromise);
-    for (const promise of promises) {
-      handler.waitUntil(promise);
-    }
 
-    // Promise.race() will resolve as soon as the first promise resolves.
-    let response = await Promise.race(promises);
-    // If Promise.race() resolved with null, it might be due to a network
-    // timeout + a cache miss. If that were to happen, we'd rather wait until
-    // the networkPromise resolves instead of returning null.
-    // Note that it's fine to await an already-resolved promise, so we don't
-    // have to check to see if it's still "in flight".
-    if (!response) {
-      response = await networkPromise;
-    }
+    const response = await handler.waitUntil((async () => {
+      // Promise.race() will resolve as soon as the first promise resolves.
+      return await handler.waitUntil(Promise.race(promises)) ||
+          // If Promise.race() resolved with null, it might be due to a network
+          // timeout + a cache miss. If that were to happen, we'd rather wait until
+          // the networkPromise resolves instead of returning null.
+          // Note that it's fine to await an already-resolved promise, so we don't
+          // have to check to see if it's still "in flight".
+          await networkPromise;
+    })());
 
     if (process.env.NODE_ENV !== 'production') {
       logger.groupCollapsed(

packages/workbox-strategies/src/StrategyHandler.ts

@@ -497,7 +497,7 @@ class StrategyHandler {
    * @param {Promise} promise A promise to add to the extend lifetime promises
    *     of the event that triggered the request.
    */
-  waitUntil(promise: Promise<any>): Promise<any> {
+  waitUntil<T>(promise: Promise<T>): Promise<T> {
     this._extendLifetimePromises.push(promise);
     return promise;
   }

test/workbox-strategies/sw/test-NetworkFirst.mjs

@@ -141,12 +141,37 @@ describe(`NetworkFirst`, function() {
       const cache = await caches.open(cacheNames.getRuntimeName());
       await cache.put(request, injectedResponse.clone());
 
-      const handlePromise = networkFirst.handle({
+      const [handlePromise, donePromise] = networkFirst.handleAll({
         request,
         event,
       });
 
-      await eventDoneWaiting(event);
+      await donePromise;
+
+      const populatedCacheResponse = await handlePromise;
+      await compareResponses(populatedCacheResponse, injectedResponse, true);
+    });
+
+    it(`should signal completion if the network request completes before timing out`, async function() {
+      const request = new Request('http://example.io/test/');
+      const event = new FetchEvent('fetch', {request});
+      spyOnEvent(event);
+
+      const networkTimeoutSeconds = 10;
+
+      const injectedResponse = new Response('response body');
+      sandbox.stub(self, 'fetch').resolves(injectedResponse);
+
+      const networkFirst = new NetworkFirst({networkTimeoutSeconds});
+
+      const [handlePromise, donePromise] = networkFirst.handleAll({
+        request,
+        event,
+      });
+
+      const startTime = performance.now();
+      await donePromise;
+      expect(performance.now() - startTime).to.be.below(1000);
 
       const populatedCacheResponse = await handlePromise;
       await compareResponses(populatedCacheResponse, injectedResponse, true);