Background sync queue callbacks #666

philipwalton · 2017-07-07T06:15:59Z

Fixes #646

This PR updates the background sync queue callbacks to address the issues brought up in #646. Here's what's changed:

renamed: onResponse => replayDidSucceed
renamed: onRetryFailure => replayDidFail
added: requestWillEnqueue
added: requestWillDequeue

I've also added a utility method to deprecate (and warn about) old object methods since I'm renaming existing callbacks, but I didn't feel like it warranted a major version bump.

jeffposnick

General approach LGTM, with a few comments.

Is there a place in the JSDoc that mentions which refers to the old onResponse/onRetryFailure names? If so, could you update that to refer to the new names? (And if not, could you add in the appropriateJSDoc for them?)

jeffposnick · 2017-07-07T18:24:52Z

packages/workbox-background-sync/src/lib/request-manager.js

+
+    // Rename deprecated callbacks.
+    const base = 'workbox-background-sync.RequestManager.callbacks';
+    deprecate(callbacks, base, 'onResponse', 'replayDidSucceed');


Could you make these calls to deprecate conditional, and only invoke each one if the corresponding callbacks.onResponse / callbacks.onRetryFailure are set?

Yeah, this was an oversight and noticed it when updating the GA stuff. It should be fixed now.

jeffposnick · 2017-07-07T18:26:23Z

packages/workbox-background-sync/src/lib/request-queue.js

    }
  }

  /**
   * get the Request from the queue at a particular index
   *
   * @param {string} hash hash of the request at the given index
-   * @return {Request} request object corresponding to given hash
+   * @return {Promise} request object corresponding to given hash


{Promise<Request>}

This doesn't fulfill to a Request instance, it fulfills to an object with config, metadata, and request properties, and the request property is a JSON-able version of the Request object.

I could update it to:

/** @return {Promise<Object>} */

But I didn't see much of that in the existing code.

my bad it is indeed

/** @return {Promise<Object>} */

philipwalton · 2017-07-07T19:42:58Z

The only callback documentation I saw in the original code was here, so that's where I updated it:

workbox/packages/workbox-background-sync/src/lib/background-sync-queue-plugin.js

Lines 10 to 18 in 1bf91ec

    
            *     replayDidSucceed: async(hash, res) => { 
        
            *       self.registration.showNotification('Background sync demo', { 
        
            *         body: 'Product has been purchased.', 
        
            *         icon: '/images/shop-icon-384.png', 
        
            *        }); 
        
            *     }, 
        
            *     replayDidFail: (hash) => {}, 
        
            *     requestWillEnqueue: (reqData) => {}, 
        
            *     requestWillDequeue: (reqData) => {},

I agree that the JSDoc comments need to be more clear, but I wasn't sure what conventions we were using for documenting sub-properties (I'm not seeing a lot of consistency, so I was going to bring it up in the next meeting).

If we're going to be following Google JS style, we should be documenting them like this (but again, I didn't see that so I'm not sure if there was a reason we weren't doing it that way, e.g. doc generation, etc.):
https://google.github.io/styleguide/jsguide.html#features-objects-destructuring

philipwalton · 2017-07-07T20:05:14Z

I was also going to suggest either I or @prateekbh do a clean up since most of the files in this project don't include the license, and there are many cases where the code doesn't match the style guide.

If I update the gulp-eslint and eslint-config-google versions I notice a lot more errors, most of which can be easily fixed via the --fix option.

But that should probably be handled in a separate PR.

jeffposnick · 2017-07-07T20:29:31Z

(I'm in the middle of putting together a PR that updates some of our linting rules, tracked in #478)

philipwalton · 2017-07-07T20:30:45Z

(I'm in the middle of putting together a PR that updates some of our linting rules, tracked in #478)

Cool, happy to help out on that front if you want.

prateekbh · 2017-07-08T05:39:45Z

If I update the gulp-eslint and eslint-config-google versions I notice a lot more errors, most of which can be easily fixed via the --fix option.

Happy to do that. I tried adding prettier, but it conflicts with our rules a little too much(would like to discuss in the next meeting)

LGTM 👍

philipwalton · 2017-07-10T19:12:00Z

I've updated the jsdoc comments. @jeffposnick PTAL.

philipwalton · 2017-07-10T20:11:25Z

One more issue:

The current workbox-google-analytics API accepts a hitFilter function, that -- if it throws -- aborts the hit and stops further retries. If we want to keep this behavior, we'd have to add an additional feature here to support aborting a retry.

The way queuing currently works, items are left in the queue even after a successful retry; they're only removed during clean up.

If we want to support stopping a request from being replayed, we should probably add a utility method to remove an item from the queue rather than depending on a clean up cycle.

I think this makes sense since I know I was momentarily confused when debugging by the fact that successful replays remained in the queue.

prateekbh · 2017-07-11T02:54:37Z

If we want to support stopping a request from being replayed, we should probably add a utility method to remove an item from the queue rather than depending on a clean up cycle

I can take this up, do you want the item to be removed after successful replay or do you have anything else in mind?

P.S. clean up removes the request from IDB after its threshold time. But we can remove the request from the current instance after a successful replay.

prateekbh · 2017-07-11T04:59:09Z

@philipwalton As far as hitFilter is concerned, maybe we can add a requestWillPlay callback much like requestWillDeque, and returning false from this can make the request skip replaying.

jeffposnick · 2017-07-11T13:48:58Z

@philipwalton, are there "legitimate" use cases for wanting to abort things via an exception in hitFilter? Or was that just an unintended side effect of the previous codebase? It sounds like it might be cleaner not to support that, and add in catch() as needed, along with logging to report the error.

jeffposnick · 2017-07-11T13:54:08Z

lib/deprecate.js

+export default (obj, oldName, newName, ctx) => {
+  if (Object.prototype.hasOwnProperty.call(obj, oldName)) {
+    /* eslint-disable no-console */
+    console.warn(`In ${ctx}: ` +


Sorry for the late comment, but I'd suggest using the warn() method of lib/log-helper here. It will keep the messages in the console consistent, and you can include some of the detailed information in the optional second parameter, like:

logHelper.warn(`${oldName} is deprecated; use ${newName} instead`, {Context: ctx});

Good to know. Done.

philipwalton · 2017-07-11T16:59:09Z

@philipwalton, are there "legitimate" use cases for wanting to abort things via an exception in hitFilter? Or was that just an unintended side effect of the previous codebase? It sounds like it might be cleaner not to support that, and add in catch() as needed, along with logging to report the error.

I'd be OK with not supporting it for the analytics use case. I originally implemented it primarily for compatibility with how tasks work in analytics.js, but I don't see it as a critical feature by any means.

@philipwalton As far as hitFilter is concerned, maybe we can add a requestWillPlay callback much like requestWillDeque, and returning false from this can make the request skip replaying.

From a generic background sync perspective, I can imagine cases where a developer wouldn't want to replay a hit. Perhaps in cases where they know the current state data is fresher than what this hit will send (though I suppose you could argue they could/should handle that on the back end).

If we add this method I'd recommend the name requestShouldReplay, and if should probably be async since you'd likely need to check IDB to know whether or not it should replay.

..in fact, perhaps all of these should be async :)

philipwalton · 2017-07-11T18:42:49Z

@prateekbh and I discussed this offline, and if the rest of the team is OK with @jeffposnick's suggestion to drop support for aborting a replay by throwing in the hitFilter function, then we think we're ready to merge as is.

We decided it's probably better to wait for an actual use case for aborting a replay and design the API around that rather than trying to anticipate one.

So if everyone agrees, I think we should merge this and move the discussion of how to handling the API change in the workbox-google-analytics PR.

jeffposnick · 2017-07-11T18:47:26Z

SGTM.

philipwalton added 4 commits July 6, 2017 22:53

Add a module to warn about deprecated methods

90cd2f0

Update/add callbacks for background sync queue

6f05e67

Fix lint errors

f190b28

Fix incorrectly named methods

9739578

philipwalton requested review from jeffposnick and prateekbh July 7, 2017 06:15

Fix failing test due to rename

ac3d74a

jeffposnick suggested changes Jul 7, 2017

View reviewed changes

Update deprecation untility function

1bf91ec

prateekbh approved these changes Jul 8, 2017

View reviewed changes

Update jsdoc comments per review feedback

509245d

jeffposnick suggested changes Jul 11, 2017

View reviewed changes

Use logHelper instead of console.warn

8af00af

jeffposnick approved these changes Jul 11, 2017

View reviewed changes

philipwalton added 2 commits July 11, 2017 12:29

Merge branch 'master' into background-sync-queue-callbacks

1827413

Fix post-merge lint errors

21f7ad2

philipwalton merged commit d60bf97 into master Jul 11, 2017

philipwalton deleted the background-sync-queue-callbacks branch July 11, 2017 20:07

snyk-bot mentioned this pull request May 29, 2020

[Snyk] Security upgrade firebase-functions from 0.7.0 to 3.6.2 Jeremip11/workbox#5

Open

Jeremip11 mentioned this pull request Dec 22, 2022

[Snyk] Security upgrade firebase-functions from 0.7.0 to 3.6.2 Jeremip11/workbox#20

Open

donavon mentioned this pull request Dec 22, 2022

[Snyk] Fix for 4 vulnerabilities donavon/workbox#16

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Background sync queue callbacks #666

Background sync queue callbacks #666

philipwalton commented Jul 7, 2017

jeffposnick left a comment

jeffposnick Jul 7, 2017

philipwalton Jul 7, 2017

jeffposnick Jul 7, 2017

philipwalton Jul 7, 2017

prateekbh Jul 8, 2017

philipwalton commented Jul 7, 2017

philipwalton commented Jul 7, 2017 •

edited

Loading

jeffposnick commented Jul 7, 2017

philipwalton commented Jul 7, 2017

prateekbh commented Jul 8, 2017 •

edited

Loading

philipwalton commented Jul 10, 2017

philipwalton commented Jul 10, 2017

prateekbh commented Jul 11, 2017

prateekbh commented Jul 11, 2017

jeffposnick commented Jul 11, 2017

jeffposnick Jul 11, 2017

philipwalton Jul 11, 2017

philipwalton commented Jul 11, 2017

philipwalton commented Jul 11, 2017

jeffposnick commented Jul 11, 2017

Background sync queue callbacks #666

Background sync queue callbacks #666

Conversation

philipwalton commented Jul 7, 2017

jeffposnick left a comment

Choose a reason for hiding this comment

jeffposnick Jul 7, 2017

Choose a reason for hiding this comment

philipwalton Jul 7, 2017

Choose a reason for hiding this comment

jeffposnick Jul 7, 2017

Choose a reason for hiding this comment

philipwalton Jul 7, 2017

Choose a reason for hiding this comment

prateekbh Jul 8, 2017

Choose a reason for hiding this comment

philipwalton commented Jul 7, 2017

philipwalton commented Jul 7, 2017 • edited Loading

jeffposnick commented Jul 7, 2017

philipwalton commented Jul 7, 2017

prateekbh commented Jul 8, 2017 • edited Loading

philipwalton commented Jul 10, 2017

philipwalton commented Jul 10, 2017

prateekbh commented Jul 11, 2017

prateekbh commented Jul 11, 2017

jeffposnick commented Jul 11, 2017

jeffposnick Jul 11, 2017

Choose a reason for hiding this comment

philipwalton Jul 11, 2017

Choose a reason for hiding this comment

philipwalton commented Jul 11, 2017

philipwalton commented Jul 11, 2017

jeffposnick commented Jul 11, 2017

philipwalton commented Jul 7, 2017 •

edited

Loading

prateekbh commented Jul 8, 2017 •

edited

Loading