Background sync should add the ability to modify request prior to retrying

[philipwalton]

I started looking into how to update the workbox-google-analytics library to use background-sync, and I discovered that there's no way to modify the request before it retries.

For the google analytics hit to work it needs to update the qt param to the difference between the current time and the original hit time. It also (for consistency with the existing API), needs to let developers set param overrides as well as modify the existing params via a hitFilter function.

I see two options to make this work, and I wanted to get feedback before making any changes to the background sync code:

1. Add a new callback to the workbox.backgroundSync.Queue so it can modify the request after it pulls it from the IDB cache and before it sends it.

2. Update the workbox.backgroundSync.RequestQueue class to support queuing both plain Request objects as well as Request objects with a corresponding RequestWrapper object, then we could use a RequestWrapper plugin's requestWillFetch lifecycle method to modify the request prior to sending it.

I think both options are fine, but I slightly prefer option #2, if for no other reason than it seems wrong to duplicate behavior we already have in RequestWrapper. That being said, option #1 is probably much easier to implement.

What do others think? @jeffposnick @prateekbh

[prateekbh]

Yeah this sounds like a feature which could really help devs differentiate even normal req from the later replayed request, so 👍 for the feature.

As for the code location part, we can take a similar function in the constructor and call it here
https://github.com/GoogleChrome/workbox/blob/master/packages/workbox-background-sync/src/lib/request-manager.js#L55-L56

Although I am a little unsure if a config param override would make sense for background sync library. Is it possible that we can still keep this logic inside workbox-google-analytics and use background-sync underneath?

[jeffposnick]

I'm in favor of moving all internal uses of fetch() inside of the background sync project to use RequestWrapper.fetch() unconditionally, and to allow developers to provide their own RequestWrapper instance with a configured requestWillFetch plugin when constructing the Queue.

If they don't provide a RequestWrapper when constructing the Queue, we can just create one by default.

I can put together a PR for that, and once it's in place, @philipwalton will be able to handle what he needs by putting together a requestWillFetch plugin customized for the Google Analytics project, without needing any additional logic adding to the background sync project.

[philipwalton]

Reopening because it looks like this still isn't good enough.

The requestWillFetch lifecycle method lets me modify the request, but it doesn't give me access to any of the queue metadata (like the time of the original request), so I'm not able to set the qt param.

After spending a bit of time thinking about it, I think doing option 1 above is still necessary, though since the method required will have to return something, I wonder if it makes sense to rethink how these are named (since it's more of a handler and less of a callback).

[prateekbh]

hey @philipwalton @jeffposnick ,
not sure if this is the cleanest approach but will it work if along with the request, I can send metadata of the request attached with the request metadata in something like request.__backgroundSyncMetadata ? Then you can use this inside the requestWrapper's callback function to set the qt param.

else we can work towards an approach for option 1 of yours.

Also a note, keeping two callbacks, option 1 and request wrapper's callbacks might get a little confusing, any thoughts?

[philipwalton]

I'm not sure that's the cleanest approach either. Seems awfully specific to a particular use case and not reusable for anything else. @jeffposnick wdyt?

[prateekbh]

Makes sense, unless there is a cleaner approach i guess we can go with option 1. You wanna PR?

[jeffposnick]

Gotcha—sorry that #648 wasn't sufficient.

If you're going to write some code to implement option 1, then we can revert #648. I don't think there's a good reason to leave it in place.

[philipwalton]

You wanna PR?

Sure, I can try to throw something together and then we can bikeshed on method names later.

If you're going to write some code to implement option 1, then we can revert #648. I don't think there's a good reason to leave it in place.

SGTM