From 6c79afd01eb2f1e22548eaa7e0770b8c130ac7bc Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Thu, 4 Jan 2018 13:41:49 -0800 Subject: [PATCH 1/4] throwing error instead of re registering sync --- packages/workbox-background-sync/Queue.mjs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/workbox-background-sync/Queue.mjs b/packages/workbox-background-sync/Queue.mjs index e28063584..e9bb839f3 100644 --- a/packages/workbox-background-sync/Queue.mjs +++ b/packages/workbox-background-sync/Queue.mjs @@ -104,6 +104,8 @@ class Queue { * `queueDidReplay` callback is invoked (which implies the queue is * now empty). If any of the requests fail, a new sync registration is * created to retry again later. + * + * @return {Promise} */ async replayRequests() { const now = Date.now(); @@ -132,17 +134,17 @@ class Queue { replayedRequests.push(replay); } + await this._runCallback('queueDidReplay', replayedRequests); + // If any requests failed, put the failed requests back in the queue - // and register for another sync. + // and reject promise. if (failedRequests.length) { await Promise.all(failedRequests.map((storableRequest) => { return this._queueStore.addEntry(storableRequest); })); - await this._registerSync(); + return Promise.reject(failedRequests); } - - await this._runCallback('queueDidReplay', replayedRequests); } /** From bb0290031afb77b365622ecbf6addac0811d7991 Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Thu, 4 Jan 2018 14:43:33 -0800 Subject: [PATCH 2/4] fixing tests --- .../node/lib/test-Queue.mjs | 46 ++++++++++++------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/test/workbox-background-sync/node/lib/test-Queue.mjs b/test/workbox-background-sync/node/lib/test-Queue.mjs index 3b781d807..113fef033 100644 --- a/test/workbox-background-sync/node/lib/test-Queue.mjs +++ b/test/workbox-background-sync/node/lib/test-Queue.mjs @@ -293,37 +293,43 @@ describe(`[workbox-background-sync] Queue`, function() { await queue.addRequest(new Request('/three')); await queue.addRequest(new Request('/four')); await queue.addRequest(new Request('/five')); - await queue.replayRequests(); // The 2nd and 4th requests should fail. - - const entries = await getObjectStoreEntries(); - expect(entries.length).to.equal(2); - expect(entries[0].storableRequest.url).to.equal('/two'); - expect(entries[1].storableRequest.url).to.equal('/four'); + try { + await queue.replayRequests(); // The 2nd and 4th requests should fail. + } catch (error) { + const entries = await getObjectStoreEntries(); + expect(entries.length).to.equal(2); + expect(entries[0].storableRequest.url).to.equal('/two'); + expect(entries[1].storableRequest.url).to.equal('/four'); + return; + } + + return Promise.reject('should have exit from catch'); }); - it(`should re-register for a sync event if re-fetching fails`, + it(`should reject replayRequests promise if re-fetching fails`, async function() { - sandbox.stub(self.registration, 'sync').value({ - register: sinon.stub().resolves(), - }); sandbox.stub(self, 'fetch') .onCall(1).rejects(new Error()) .callThrough(); + const failureURL = '/two'; const queue = new Queue('foo'); // Add requests for both queues to ensure only the requests from // the matching queue are replayed. await queue.addRequest(new Request('/one')); - await queue.addRequest(new Request('/two')); + await queue.addRequest(new Request(failureURL)); - self.registration.sync.register.reset(); - await queue.replayRequests(); // The second request should fail. + try { + await queue.replayRequests(); // The second request should fail. + } catch (error) { + expect(error.length).to.be.equal(1); + expect(error[0].url).to.be.equal(failureURL); + return; + } - expect(self.registration.sync.register.calledOnce).to.be.true; - expect(self.registration.sync.register.calledWith( - 'workbox-background-sync:foo')).to.be.true; + return Promise.reject('should have exit from catch'); }); it(`should invoke all replay callbacks`, async function() { @@ -376,7 +382,13 @@ describe(`[workbox-background-sync] Queue`, function() { await queue.addRequest(new Request('/three')); await queue.addRequest(new Request('/four')); - await queue.replayRequests(); + try { + await queue.replayRequests(); + } catch (error) { + // Dont do anything as this is expected to be rejected + // but every callback should still be called. + } + expect(requestWillReplay.calledTwice).to.be.true; From 6c480885b65d7a9908bd8ac880078d2a5c657e4d Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Fri, 5 Jan 2018 11:30:01 -0800 Subject: [PATCH 3/4] addressing promises --- packages/workbox-background-sync/Queue.mjs | 7 +++---- packages/workbox-core/models/messages/messages.mjs | 4 ++++ test/workbox-background-sync/node/lib/test-Queue.mjs | 10 +++++----- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/packages/workbox-background-sync/Queue.mjs b/packages/workbox-background-sync/Queue.mjs index e9bb839f3..aac1eadca 100644 --- a/packages/workbox-background-sync/Queue.mjs +++ b/packages/workbox-background-sync/Queue.mjs @@ -104,8 +104,6 @@ class Queue { * `queueDidReplay` callback is invoked (which implies the queue is * now empty). If any of the requests fail, a new sync registration is * created to retry again later. - * - * @return {Promise} */ async replayRequests() { const now = Date.now(); @@ -137,13 +135,14 @@ class Queue { await this._runCallback('queueDidReplay', replayedRequests); // If any requests failed, put the failed requests back in the queue - // and reject promise. + // and rethrow the failed requests count. if (failedRequests.length) { await Promise.all(failedRequests.map((storableRequest) => { return this._queueStore.addEntry(storableRequest); })); - return Promise.reject(failedRequests); + throw new WorkboxError('queue-replay-failed', + {name: this._name, count: failedRequests.length}); } } diff --git a/packages/workbox-core/models/messages/messages.mjs b/packages/workbox-core/models/messages/messages.mjs index 686545d24..3a9bc8c10 100644 --- a/packages/workbox-core/models/messages/messages.mjs +++ b/packages/workbox-core/models/messages/messages.mjs @@ -128,6 +128,10 @@ export default { `registered.`; }, + 'queue-replay-failed': ({name, count}) => { + return `${count} requests failed, while trying to replay Queue: ${name}.`; + }, + 'duplicate-queue-name': ({name}) => { return `The Queue name '${name}' is already being used. ` + `All instances of backgroundSync.Queue must be given unique names.`; diff --git a/test/workbox-background-sync/node/lib/test-Queue.mjs b/test/workbox-background-sync/node/lib/test-Queue.mjs index 113fef033..4999d0f63 100644 --- a/test/workbox-background-sync/node/lib/test-Queue.mjs +++ b/test/workbox-background-sync/node/lib/test-Queue.mjs @@ -23,6 +23,7 @@ import {DB_NAME, OBJECT_STORE_NAME} from import {DBWrapper} from '../../../../packages/workbox-core/_private/DBWrapper.mjs'; import {resetEventListeners} from '../../../../infra/testing/sw-env-mocks/event-listeners.js'; +import {WorkboxError} from '../../../../packages/workbox-core/_private/WorkboxError.mjs'; const getObjectStoreEntries = async () => { return await new DBWrapper(DB_NAME, 1).getAll(OBJECT_STORE_NAME); @@ -304,10 +305,10 @@ describe(`[workbox-background-sync] Queue`, function() { return; } - return Promise.reject('should have exit from catch'); + throw new Error('should have exit from catch'); }); - it(`should reject replayRequests promise if re-fetching fails`, + it(`should throw WorkboxError if re-fetching fails`, async function() { sandbox.stub(self, 'fetch') .onCall(1).rejects(new Error()) @@ -324,12 +325,11 @@ describe(`[workbox-background-sync] Queue`, function() { try { await queue.replayRequests(); // The second request should fail. } catch (error) { - expect(error.length).to.be.equal(1); - expect(error[0].url).to.be.equal(failureURL); + expect(error).to.be.instanceof(WorkboxError); return; } - return Promise.reject('should have exit from catch'); + throw new Error('should have exit from catch'); }); it(`should invoke all replay callbacks`, async function() { From 274bb31f24df39c14d682fb98d1a31d38cf4beaa Mon Sep 17 00:00:00 2001 From: Prateek Bhatnagar Date: Fri, 5 Jan 2018 15:31:14 -0800 Subject: [PATCH 4/4] using expect error --- .../node/lib/test-Queue.mjs | 41 +++++++------------ 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/test/workbox-background-sync/node/lib/test-Queue.mjs b/test/workbox-background-sync/node/lib/test-Queue.mjs index 4999d0f63..61bf1af1e 100644 --- a/test/workbox-background-sync/node/lib/test-Queue.mjs +++ b/test/workbox-background-sync/node/lib/test-Queue.mjs @@ -23,7 +23,6 @@ import {DB_NAME, OBJECT_STORE_NAME} from import {DBWrapper} from '../../../../packages/workbox-core/_private/DBWrapper.mjs'; import {resetEventListeners} from '../../../../infra/testing/sw-env-mocks/event-listeners.js'; -import {WorkboxError} from '../../../../packages/workbox-core/_private/WorkboxError.mjs'; const getObjectStoreEntries = async () => { return await new DBWrapper(DB_NAME, 1).getAll(OBJECT_STORE_NAME); @@ -295,17 +294,14 @@ describe(`[workbox-background-sync] Queue`, function() { await queue.addRequest(new Request('/four')); await queue.addRequest(new Request('/five')); - try { - await queue.replayRequests(); // The 2nd and 4th requests should fail. - } catch (error) { - const entries = await getObjectStoreEntries(); - expect(entries.length).to.equal(2); - expect(entries[0].storableRequest.url).to.equal('/two'); - expect(entries[1].storableRequest.url).to.equal('/four'); - return; - } - - throw new Error('should have exit from catch'); + await expectError(() => { + return queue.replayRequests(); // The 2nd and 4th requests should fail. + }, 'queue-replay-failed'); + + const entries = await getObjectStoreEntries(); + expect(entries.length).to.equal(2); + expect(entries[0].storableRequest.url).to.equal('/two'); + expect(entries[1].storableRequest.url).to.equal('/four'); }); it(`should throw WorkboxError if re-fetching fails`, @@ -322,14 +318,9 @@ describe(`[workbox-background-sync] Queue`, function() { await queue.addRequest(new Request('/one')); await queue.addRequest(new Request(failureURL)); - try { - await queue.replayRequests(); // The second request should fail. - } catch (error) { - expect(error).to.be.instanceof(WorkboxError); - return; - } - - throw new Error('should have exit from catch'); + await expectError(() => { + return queue.replayRequests(); + }, 'queue-replay-failed'); }); it(`should invoke all replay callbacks`, async function() { @@ -382,13 +373,9 @@ describe(`[workbox-background-sync] Queue`, function() { await queue.addRequest(new Request('/three')); await queue.addRequest(new Request('/four')); - try { - await queue.replayRequests(); - } catch (error) { - // Dont do anything as this is expected to be rejected - // but every callback should still be called. - } - + await expectError(() => { + return queue.replayRequests(); + }, 'queue-replay-failed'); expect(requestWillReplay.calledTwice).to.be.true;