diff --git a/packages/workbox-strategies/src/NetworkFirst.ts b/packages/workbox-strategies/src/NetworkFirst.ts index 57d3982f2..173d09642 100644 --- a/packages/workbox-strategies/src/NetworkFirst.ts +++ b/packages/workbox-strategies/src/NetworkFirst.ts @@ -100,7 +100,7 @@ class NetworkFirst extends Strategy { }); } - const promises = []; + const promises: Promise[] = []; 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( diff --git a/packages/workbox-strategies/src/StrategyHandler.ts b/packages/workbox-strategies/src/StrategyHandler.ts index b1ce5e484..ebf54a1b9 100644 --- a/packages/workbox-strategies/src/StrategyHandler.ts +++ b/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): Promise { + waitUntil(promise: Promise): Promise { this._extendLifetimePromises.push(promise); return promise; } diff --git a/test/workbox-strategies/sw/test-NetworkFirst.mjs b/test/workbox-strategies/sw/test-NetworkFirst.mjs index b1d19d06b..272674f3f 100644 --- a/test/workbox-strategies/sw/test-NetworkFirst.mjs +++ b/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);