Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throwing error instead of re registering sync on failure #1155

Merged
merged 5 commits into from
Jan 8, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/workbox-background-sync/Queue.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reword this to: "and rethrow the failed requests"?

Note: this comment I'm less concerned about changing...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (failedRequests.length) {
await Promise.all(failedRequests.map((storableRequest) => {
return this._queueStore.addEntry(storableRequest);
}));

await this._registerSync();
return Promise.reject(failedRequests);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can we throw here instead of returning a rejected promise? I think it's better to use async function patterns when inside an async function (rather that promise patterns).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so throw new Error(failedRequests);?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking just:

throw failedRequests;

In JS you can throw any value.

cc: @gauntface and @jeffposnick to see if they have thoughts on this.

In general I think we should stick to the pattern that async function use await, try, catch, and throw, and regular functions use resolve and reject, though I guess this may be a bit of a special case...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anything end up consuming the rejected promise that's returned by this function? Or is the fact that it's a rejected promise all that matters, with the rejection value ignored?

If the value is ignored, then I'd rather see

throw new Error('<some meaningful message>');

so that a developer reading the source has a bit more context. And if at some point in the future logging is added in, it will end up with a meaningful log message.

If the value isn't ignored, and it needs to be that array, then using Promise.reject() seems fine to me, but I'd ask that you add in a comment explaining what's going on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah nothing really consumes it.

  • i was thinking something likethrow new Error('X number of requests failed');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a WorkboxError.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

await this._runCallback('queueDidReplay', replayedRequests);
}

/**
Expand Down
46 changes: 29 additions & 17 deletions test/workbox-background-sync/node/lib/test-Queue.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, can we throw here instead of returning a rejected promise?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

});

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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, can we throw here instead of returning a rejected promise?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

});

it(`should invoke all replay callbacks`, async function() {
Expand Down Expand Up @@ -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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use expectError here and make sure the error is what you expect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

} 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;

Expand Down