Add requestDidReplay callback to BackgroundSync Queue (#1502)

[Toilal]

R: @jeffposnick @philipwalton

Fixes #1502

Adds requestDidReplay and queueWillReplay callback

[coveralls]

Coverage increased (+0.006%) to 86.085% when pulling 5aedcbb on Toilal:request-did-replay into 59e7d4f on GoogleChrome:master.

[philipwalton]

Thanks @Toilal for the explanation in #1502. However, from the use case you described, it sounds like the proposed changes still might not work for you.

The changes you've submitted allows for modifying requests, but it doesn't allow for inserting requests into the queue before making further requests. Is that going to be a problem for you?

At the moment you can add new requests via the addRequests() method, but that will add them to the end of the queue. If you need them to fire in-between replayed requests, I think you'd have to make them manually.

[Toilal]

To implement the use case in #1502, when a requests fail, we have to set a global let hasFailed= true to force an error on all following requests to be retried (throw new Error()). Then all requests will be requeued in the original order.

It seems like a hack, it's sad that the API is not more opened to implement this use case properly.

[philipwalton]

@Toilal I agree, which is why I wanted to make sure your use case was being met before just merging this change. When we designed this API, we didn't have a lot of use cases in mind, so I'm happy to rework it to make it more flexible.

Do you have any ideas?

One option -- which would be the most flexible but also the biggest pain for users to implement -- is to provide raw access to the request queue. This would allow users to manually retry all requests however they way, and if any fail they can then manually add them back to the queue. What do you think?

[Toilal]

To cover more real use cases.

• Both callbacks added by this pull requests (queueWillReplay and requestDidReplay).

• A way to break the current replay loop. A loop break could be introduced when requestDidReplay or requestWillReplay returns false. When the loop is interrupted as such, the current request beeing replayed should be restored in the queue at its original position (last). It should still be the first element of the loop when the queue is replayed the next time. In many case, we need to garantee the requests are replayed in order, and don't wan't to go to next one when something goes wrong.

• A way to mutate stored requests (to refresh Authorization header for example, when JWT token is expired).

queue.forEach((request) => doMutate(request))

[vcastro45]

I allow myself to suggest an idea that goes through my mind:

• It would also be useful to extend the "case of fail". In fact, currently a request is queued if the fetch() call throw an error but sometimes this fetch() call is successful but the request has not been handled properly by the server. For example, if the server returns a HTTP Code 401 (when the JWT refresh token is expired), we would like to be able to consider the request as failed and renew the token on the next queueWillReplay

Of course, this example is for code 401 but it can be adapted for any code (>= 400 ?)

[fomenyesu]

Totally agree with you. @Toilal
What you said is a normal use case in the real world of background sync.

[bhargashah]

Since this work is approved, can this be merged soon?

[Toilal]

I have rebased this pull request on current master version (3.6.2), and all checks are now OK.

Please consider merging this one. I'm using it since months at it may solve many use cases.

[philipwalton]

Hey all, we're going to address this issue as part of our next major release (since it may affect the public API). We're still exploring whether this PR is sufficient or whether it requires a larger change to handle some of the use cases we've seen.

[WadeFanyao]

Hi everyone. I have a simple case and I am not sure the next release will help. For example, I have a get request to get data and a post request to send the data to server. The post request needs the data from the get request. To my understanding, I can implement it with callback or promises when it is online. But I just wondering if the workbox-background-sync(Next release) can queue them together and run behind when the connection is back.

[vcastro45]

@WadeFanyao I've done it using the requestDidReplay() implementation but it is painful.
In fact, if you want to do it, you have to use temporary datas to link your requests (like temporary negative IDs) and loop through each pending request when a request is replayed to assign the new datas (generated by the server) to the upcoming request using temporary datas before they are sent to the server.

If you didn't understand, no worry, it's because it's awful.

[philipwalton]

Hey all, I'm submitted #1710, which I believe will address all of the use cases discussed here. Please take a look at that PR and let me know what you think.

[philipwalton]

Closing in favor of #1710.