From c956f6ccc1a5afd55815606fb200b181f5935738 Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Wed, 26 Aug 2020 15:06:56 -0400 Subject: [PATCH 1/3] networkTimeoutSeconds in NetworkOnly --- .../workbox-strategies/src/NetworkOnly.ts | 52 ++++++++++++++++--- .../sw/test-NetworkOnly.mjs | 29 +++++++++++ 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/packages/workbox-strategies/src/NetworkOnly.ts b/packages/workbox-strategies/src/NetworkOnly.ts index 97d27b776..0b7b5127b 100644 --- a/packages/workbox-strategies/src/NetworkOnly.ts +++ b/packages/workbox-strategies/src/NetworkOnly.ts @@ -10,12 +10,16 @@ import {assert} from 'workbox-core/_private/assert.js'; import {logger} from 'workbox-core/_private/logger.js'; import {WorkboxError} from 'workbox-core/_private/WorkboxError.js'; -import {Strategy} from './Strategy.js'; +import {Strategy, StrategyOptions} from './Strategy.js'; import {StrategyHandler} from './StrategyHandler.js'; import {messages} from './utils/messages.js'; import './_version.js'; +interface NetworkOnlyOptions extends Omit { + networkTimeoutSeconds?: number; +} + /** * An implementation of a * [network-only]{@link https://developers.google.com/web/fundamentals/instant-and-offline/offline-cookbook/#network-only} @@ -30,6 +34,28 @@ import './_version.js'; * @memberof module:workbox-strategies */ class NetworkOnly extends Strategy { + private readonly _networkTimeoutSeconds: number; + + /** + * @param {Object} options + * @param {Array} options.plugins [Plugins]{@link https://developers.google.com/web/tools/workbox/guides/using-plugins} + * to use in conjunction with this caching strategy. + * @param {Object} options.fetchOptions Values passed along to the + * [`init`](https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch#Parameters) + * of all fetch() requests made by this strategy. + * @param {number} options.networkTimeoutSeconds If set, any network requests + * that fail to respond within the timeout will fallback to the cache. + * + * This option can be used to combat + * "[lie-fi]{@link https://developers.google.com/web/fundamentals/performance/poor-connectivity/#lie-fi}" + * scenarios. + */ + constructor(options: NetworkOnlyOptions = {}) { + super(options); + + this._networkTimeoutSeconds = options.networkTimeoutSeconds || 0; + } + /** * @private * @param {Request|string} request A request to run this strategy for. @@ -42,15 +68,29 @@ class NetworkOnly extends Strategy { assert!.isInstance(request, Request, { moduleName: 'workbox-strategies', className: this.constructor.name, - funcName: 'handle', + funcName: '_handle', paramName: 'request', }); } - let error; - let response; + let error: Error | undefined = undefined; + let response: Response | undefined; + try { - response = await handler.fetch(request); + const promises: Promise[] = [handler.fetch(request)]; + + if (this._networkTimeoutSeconds) { + const timeoutPromise = new Promise((resolve: (_: undefined) => void) => { + setTimeout(resolve, this._networkTimeoutSeconds * 1000); + }); + promises.push(timeoutPromise); + } + + response = await Promise.race(promises); + if (!response) { + throw new Error(`Timed out the network response after ` + + `${this._networkTimeoutSeconds} seconds.`); + } } catch (err) { error = err; } @@ -74,4 +114,4 @@ class NetworkOnly extends Strategy { } } -export {NetworkOnly}; +export {NetworkOnly, NetworkOnlyOptions}; diff --git a/test/workbox-strategies/sw/test-NetworkOnly.mjs b/test/workbox-strategies/sw/test-NetworkOnly.mjs index c87474a0d..ce43755c6 100644 --- a/test/workbox-strategies/sw/test-NetworkOnly.mjs +++ b/test/workbox-strategies/sw/test-NetworkOnly.mjs @@ -10,6 +10,7 @@ import {cacheNames} from 'workbox-core/_private/cacheNames.mjs'; import {NetworkOnly} from 'workbox-strategies/NetworkOnly.mjs'; import {spyOnEvent} from '../../../infra/testing/helpers/extendable-event-utils.mjs'; import {generateUniqueResponse} from '../../../infra/testing/helpers/generateUniqueResponse.mjs'; +import {sleep} from '../../../infra/testing/helpers/sleep.mjs'; describe(`NetworkOnly`, function() { @@ -138,5 +139,33 @@ describe(`NetworkOnly`, function() { expect(fetchStub.calledOnce).to.be.true; expect(fetchStub.calledWith(request, fetchOptions)).to.be.true; }); + + it(`should throw if the network request times out`, async function() { + const request = new Request('http://example.io/test/'); + const event = new FetchEvent('fetch', {request}); + spyOnEvent(event); + + // Use a short timeout to not slow down the test. + // Note Sinon fake timers do not work with `await timeout()` used + // in the current `StrategyHandler` implementation. + const networkTimeoutSeconds = 0.5; + const sleepLongerThanNetworkTimeout = + sleep(2 * networkTimeoutSeconds * 1000); + + sandbox.stub(self, 'fetch').callsFake(async () => { + await sleepLongerThanNetworkTimeout; + return new Response('Unexpected Response'); + }); + + const networkOnly = new NetworkOnly({networkTimeoutSeconds}); + + await expectError( + () => networkOnly.handle({event, request}), + 'no-response', + (err) => { + expect(err.details.error.message).to.include(networkTimeoutSeconds); + }, + ); + }); }); }); From 3c0f432c529b4c7159811fc504ee9a50bf8d1685 Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Wed, 26 Aug 2020 15:08:18 -0400 Subject: [PATCH 2/3] Linting --- test/workbox-strategies/sw/test-NetworkOnly.mjs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/workbox-strategies/sw/test-NetworkOnly.mjs b/test/workbox-strategies/sw/test-NetworkOnly.mjs index ce43755c6..9adbd4a9b 100644 --- a/test/workbox-strategies/sw/test-NetworkOnly.mjs +++ b/test/workbox-strategies/sw/test-NetworkOnly.mjs @@ -160,11 +160,11 @@ describe(`NetworkOnly`, function() { const networkOnly = new NetworkOnly({networkTimeoutSeconds}); await expectError( - () => networkOnly.handle({event, request}), - 'no-response', - (err) => { - expect(err.details.error.message).to.include(networkTimeoutSeconds); - }, + () => networkOnly.handle({event, request}), + 'no-response', + (err) => { + expect(err.details.error.message).to.include(networkTimeoutSeconds); + }, ); }); }); From 6b47f9d6ffa19ee768bf485ed981f41ddfc37ab1 Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Wed, 7 Oct 2020 16:19:45 -0400 Subject: [PATCH 3/3] Review feedback --- .../workbox-strategies/src/NetworkOnly.ts | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/packages/workbox-strategies/src/NetworkOnly.ts b/packages/workbox-strategies/src/NetworkOnly.ts index fce81c864..1c0bfbbba 100644 --- a/packages/workbox-strategies/src/NetworkOnly.ts +++ b/packages/workbox-strategies/src/NetworkOnly.ts @@ -8,6 +8,7 @@ import {assert} from 'workbox-core/_private/assert.js'; import {logger} from 'workbox-core/_private/logger.js'; +import {timeout} from 'workbox-core/_private/timeout.js'; import {WorkboxError} from 'workbox-core/_private/WorkboxError.js'; import {Strategy, StrategyOptions} from './Strategy.js'; @@ -37,18 +38,14 @@ class NetworkOnly extends Strategy { private readonly _networkTimeoutSeconds: number; /** - * @param {Object} options - * @param {Array} options.plugins [Plugins]{@link https://developers.google.com/web/tools/workbox/guides/using-plugins} + * @param {Object} [options] + * @param {Array} [options.plugins] [Plugins]{@link https://developers.google.com/web/tools/workbox/guides/using-plugins} * to use in conjunction with this caching strategy. - * @param {Object} options.fetchOptions Values passed along to the + * @param {Object} [options.fetchOptions] Values passed along to the * [`init`](https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch#Parameters) * of all fetch() requests made by this strategy. - * @param {number} options.networkTimeoutSeconds If set, any network requests - * that fail to respond within the timeout will fallback to the cache. - * - * This option can be used to combat - * "[lie-fi]{@link https://developers.google.com/web/fundamentals/performance/poor-connectivity/#lie-fi}" - * scenarios. + * @param {number} [options.networkTimeoutSeconds] If set, any network requests + * that fail to respond within the timeout will result in a network error. */ constructor(options: NetworkOnlyOptions = {}) { super(options); @@ -80,9 +77,7 @@ class NetworkOnly extends Strategy { const promises: Promise[] = [handler.fetch(request)]; if (this._networkTimeoutSeconds) { - const timeoutPromise = new Promise((resolve: (_: undefined) => void) => { - setTimeout(resolve, this._networkTimeoutSeconds * 1000); - }); + const timeoutPromise = timeout(this._networkTimeoutSeconds * 1000) as Promise; promises.push(timeoutPromise); }