Throwing error instead of re registering sync on failure #1155

prateekbh · 2018-01-04T21:35:10Z

R: @philipwalton
CC: @jeffposnick @addyosmani @gauntface

Currently, on any failures in queue replay it results in re-registering of sync queue tag.
This causes infinite loop in case of offline from devtools + IIRC will not re-play the failed requests as this will re-register the same tag and also resolve the promise inside event.waitUntil of sync event. Hence telling the browser that we've successfully done everything related to this tag.

Instead to indicate any failure it is require that the promise passed to event.waitUntil should be rejected. ref: https://github.com/WICG/BackgroundSync/blob/master/explainer.md#the-api

The promise passed to waitUntil is a signal to the user agent that the sync event is ongoing and that it should keep the service worker alive if possible. Rejection of the event signals to the user agent that the sync failed. Upon rejection the user agent should reschedule (likely with a user agent determined backoff).

…yncRetryFix

coveralls · 2018-01-04T22:53:46Z

Changes Unknown when pulling 2e6c392 on bgSyncRetryFix into ** on v3**.

philipwalton · 2018-01-04T23:41:35Z

packages/workbox-background-sync/Queue.mjs

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

-      await this._registerSync();
+      return Promise.reject(failedRequests);


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).

so throw new Error(failedRequests);?

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...

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.

yeah nothing really consumes it.

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

Please use a WorkboxError.

philipwalton · 2018-01-04T23:42:04Z

test/workbox-background-sync/node/lib/test-Queue.mjs

+        return;
+      }
+
+      return Promise.reject('should have exit from catch');


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

philipwalton · 2018-01-04T23:43:07Z

test/workbox-background-sync/node/lib/test-Queue.mjs

-      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');


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

philipwalton · 2018-01-04T23:44:50Z

packages/workbox-background-sync/Queue.mjs

    // If any requests failed, put the failed requests back in the queue
-    // and register for another sync.
+    // and reject promise.


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

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

prateekbh · 2018-01-05T00:28:48Z

@philipwalton correct me if I am wrong, but throw new Error... can only take string as param. So tops we can pass back is how many requests failed. and not the actual requests

philipwalton · 2018-01-05T00:38:19Z

Plain Error objects can only take a string, but AFAIK in JS you can throw any value. See #1155 (comment) for my comments.

gauntface · 2018-01-05T18:10:29Z

As Jeff pointed out - we should be using useful information to explain what is happening if a developer is viewing this.

The scenario you described sounds like an expected and valid scenario (i.e. the request failed to work - no worries, we'll still have the sync set up). For this - use a WorkboxError with a valid string to explain what has happened.

This means the developer will know the error is from Workbox (because it's a WorkboxError) and the message will highlight this is expected and not to worry.

coveralls · 2018-01-05T19:40:39Z

Changes Unknown when pulling 6c48088 on bgSyncRetryFix into ** on v3**.

coveralls · 2018-01-05T19:46:23Z

Changes Unknown when pulling 6c48088 on bgSyncRetryFix into ** on v3**.

gauntface · 2018-01-05T22:19:55Z

test/workbox-background-sync/node/lib/test-Queue.mjs

+        return;
+      }
+
+      throw new Error('should have exit from catch');


Could you use the helper: https://github.com/GoogleChrome/workbox/blob/v3/infra/testing/expectError.js

This will check the error is a WorkboxError with an expected code and it returns a promise, so you can await for the response and check the object store entries after.

gauntface · 2018-01-05T22:20:13Z

test/workbox-background-sync/node/lib/test-Queue.mjs

+      try {
+        await queue.replayRequests(); // The second request should fail.
+      } catch (error) {
+        expect(error).to.be.instanceof(WorkboxError);


Please use expectError helper here as well.

gauntface · 2018-01-05T22:20:38Z

test/workbox-background-sync/node/lib/test-Queue.mjs

-      expect(self.registration.sync.register.calledOnce).to.be.true;
-      expect(self.registration.sync.register.calledWith(
-          'workbox-background-sync:foo')).to.be.true;
+      throw new Error('should have exit from catch');


This wont be needed after switching to expectError().

gauntface · 2018-01-05T22:21:06Z

test/workbox-background-sync/node/lib/test-Queue.mjs

@@ -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();


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

gauntface

Could you switch to expectError() helper throughout the tests to ensure everything runs as we expect after future changes.

workbox-pr-bot · 2018-01-05T23:41:42Z

PR-Bot Size Plugin

Changed File Sizes

File	Before	After	Change	GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js	3.15 KB	3.21 KB	+2%	1.38 KB
packages/workbox-broadcast-cache-update/build/workbox-broadcast-cache-update.prod.js	1.06 KB	1.06 KB	-0%	565 B
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js	3.27 KB	3.23 KB	-1%	1.20 KB
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js	591 B	590 B	-0%	343 B
packages/workbox-core/build/workbox-core.prod.js	6.38 KB	6.36 KB	-0%	2.52 KB
packages/workbox-google-analytics/build/workbox-google-analytics.prod.js	1.92 KB	1.92 KB	-0%	999 B
packages/workbox-precaching/build/workbox-precaching.prod.js	5.18 KB	5.18 KB	-0%	2.02 KB
packages/workbox-range-requests/build/workbox-range-requests.prod.js	1.66 KB	1.65 KB	-0%	814 B
packages/workbox-routing/build/workbox-routing.prod.js	2.77 KB	2.74 KB	-1%	1.24 KB
packages/workbox-strategies/build/workbox-strategies.prod.js	3.24 KB	3.23 KB	-0%	1.00 KB
packages/workbox-sw/build/workbox-sw.js	1.46 KB	1.46 KB	-0%	794 B

New Files

No new files have been added.

All File Sizes

View Table

File	Before	After	Change	GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js	3.15 KB	3.21 KB	+2%	1.38 KB
packages/workbox-broadcast-cache-update/build/workbox-broadcast-cache-update.prod.js	1.06 KB	1.06 KB	-0%	565 B
packages/workbox-build/build/_types.js	41 B	41 B	0%	61 B
packages/workbox-build/build/index.js	2.52 KB	2.52 KB	0%	1.06 KB
packages/workbox-cache-expiration/build/workbox-cache-expiration.prod.js	3.27 KB	3.23 KB	-1%	1.20 KB
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js	591 B	590 B	-0%	343 B
packages/workbox-cli/build/app.js	4.57 KB	4.57 KB	0%	1.65 KB
packages/workbox-cli/build/bin.js	2.53 KB	2.53 KB	0%	1.07 KB
packages/workbox-core/build/workbox-core.prod.js	6.38 KB	6.36 KB	-0%	2.52 KB
packages/workbox-google-analytics/build/workbox-google-analytics.prod.js	1.92 KB	1.92 KB	-0%	999 B
packages/workbox-precaching/build/workbox-precaching.prod.js	5.18 KB	5.18 KB	-0%	2.02 KB
packages/workbox-range-requests/build/workbox-range-requests.prod.js	1.66 KB	1.65 KB	-0%	814 B
packages/workbox-routing/build/workbox-routing.prod.js	2.77 KB	2.74 KB	-1%	1.24 KB
packages/workbox-strategies/build/workbox-strategies.prod.js	3.24 KB	3.23 KB	-0%	1.00 KB
packages/workbox-sw/build/workbox-sw.js	1.46 KB	1.46 KB	-0%	794 B
packages/workbox-webpack-plugin/build/generate-sw.js	5.84 KB	5.84 KB	0%	2.04 KB
packages/workbox-webpack-plugin/build/index.js	742 B	742 B	0%	470 B
packages/workbox-webpack-plugin/build/inject-manifest.js	7.95 KB	7.95 KB	0%	2.65 KB

Workbox Aggregate Size Plugin

☠️ WARNING ☠️

We are using 152% of our max size budget.

Total Size: 22.27KB
Percentage of Size Used: 152%

Gzipped: 8.9KB

coveralls · 2018-01-05T23:42:07Z

Changes Unknown when pulling 274bb31 on bgSyncRetryFix into ** on v3**.

throwing error instead of re registering sync

6c79afd

prateekbh force-pushed the bgSyncRetryFix branch from 33798f5 to 6c79afd Compare January 4, 2018 21:42

prateekbh added 2 commits January 4, 2018 14:43

fixing tests

bb02900

Merge branch 'v3' of https://github.com/GoogleChrome/workbox into bgS…

2e6c392

…yncRetryFix

prateekbh requested a review from philipwalton January 4, 2018 22:52

philipwalton requested changes Jan 4, 2018

View reviewed changes

prateekbh force-pushed the bgSyncRetryFix branch from 68fb011 to 2e6c392 Compare January 5, 2018 00:21

gauntface added the Chillin' Not being actively worked on, or deferred for a point in the future. label Jan 5, 2018

gauntface removed the Chillin' Not being actively worked on, or deferred for a point in the future. label Jan 5, 2018

addressing promises

6c48088

prateekbh force-pushed the bgSyncRetryFix branch from 9e6125f to 6c48088 Compare January 5, 2018 19:34

philipwalton approved these changes Jan 5, 2018

View reviewed changes

prateekbh changed the title ~~[WIP] Throwing error instead of re registering sync on failure~~ Throwing error instead of re registering sync on failure Jan 5, 2018

gauntface reviewed Jan 5, 2018

View reviewed changes

gauntface suggested changes Jan 5, 2018

View reviewed changes

using expect error

274bb31

gauntface approved these changes Jan 8, 2018

View reviewed changes

gauntface merged commit 36e99bd into v3 Jan 8, 2018

gauntface deleted the bgSyncRetryFix branch January 8, 2018 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throwing error instead of re registering sync on failure #1155

Throwing error instead of re registering sync on failure #1155

prateekbh commented Jan 4, 2018 •

edited

Loading

coveralls commented Jan 4, 2018

philipwalton Jan 4, 2018

prateekbh Jan 4, 2018

philipwalton Jan 5, 2018

jeffposnick Jan 5, 2018

prateekbh Jan 5, 2018

gauntface Jan 5, 2018

prateekbh Jan 5, 2018

philipwalton Jan 4, 2018

prateekbh Jan 5, 2018

philipwalton Jan 4, 2018

prateekbh Jan 5, 2018

philipwalton Jan 4, 2018

prateekbh Jan 5, 2018

prateekbh commented Jan 5, 2018

philipwalton commented Jan 5, 2018

gauntface commented Jan 5, 2018

coveralls commented Jan 5, 2018

coveralls commented Jan 5, 2018

gauntface Jan 5, 2018

prateekbh Jan 5, 2018

gauntface Jan 5, 2018

prateekbh Jan 5, 2018

gauntface Jan 5, 2018

prateekbh Jan 5, 2018

gauntface Jan 5, 2018

prateekbh Jan 5, 2018

gauntface left a comment

workbox-pr-bot commented Jan 5, 2018

coveralls commented Jan 5, 2018

Throwing error instead of re registering sync on failure #1155

Throwing error instead of re registering sync on failure #1155

Conversation

prateekbh commented Jan 4, 2018 • edited Loading

coveralls commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekbh commented Jan 5, 2018

philipwalton commented Jan 5, 2018

gauntface commented Jan 5, 2018

coveralls commented Jan 5, 2018

coveralls commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface left a comment

Choose a reason for hiding this comment

workbox-pr-bot commented Jan 5, 2018

PR-Bot Size Plugin

Changed File Sizes

New Files

All File Sizes

Workbox Aggregate Size Plugin

☠️ WARNING ☠️

coveralls commented Jan 5, 2018

prateekbh commented Jan 4, 2018 •

edited

Loading