Fix a background sync reply bug

[philipwalton]

R: @jeffposnick

Fixes #1984

The issue in #1984 was the request wasn't being cloned in the re-fetch() attempt, which in some cases (depending on the request method and body) was causes issues when attempting to clone it later when re-adding it to the queue.

This PR fixes the issue and updates the tests to manually read the request when re-fetching to ensure this kind of thing would be caught in the future.

[jeffposnick]

👍

[workbox-pr-bot]

PR-Bot Size Plugin

Changed File Sizes

File	Before	After	Change	GZipped
packages/workbox-background-sync/build/workbox-background-sync.prod.js	3.40 KB	3.41 KB	+0%	1.48 KB

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.40 KB	3.41 KB	+0%	1.48 KB
packages/workbox-broadcast-update/build/workbox-broadcast-update.prod.js	1.89 KB	1.89 KB	0%	944 B
packages/workbox-build/build/_types.js	41 B	41 B	0%	61 B
packages/workbox-build/build/index.js	3.64 KB	3.64 KB	0%	1.36 KB
packages/workbox-cacheable-response/build/workbox-cacheable-response.prod.js	579 B	579 B	0%	344 B
packages/workbox-cli/build/app.js	5.58 KB	5.58 KB	0%	1.98 KB
packages/workbox-cli/build/bin.js	1.16 KB	1.16 KB	0%	580 B
packages/workbox-core/build/workbox-core.prod.js	5.75 KB	5.75 KB	0%	2.41 KB
packages/workbox-expiration/build/workbox-expiration.prod.js	2.89 KB	2.89 KB	0%	1.25 KB
packages/workbox-google-analytics/build/workbox-offline-ga.prod.js	1.89 KB	1.89 KB	0%	898 B
packages/workbox-navigation-preload/build/workbox-navigation-preload.prod.js	652 B	652 B	0%	317 B
packages/workbox-precaching/build/workbox-precaching.prod.js	4.24 KB	4.24 KB	0%	1.70 KB
packages/workbox-range-requests/build/workbox-range-requests.prod.js	1.51 KB	1.51 KB	0%	758 B
packages/workbox-routing/build/workbox-routing.prod.js	3.40 KB	3.40 KB	0%	1.48 KB
packages/workbox-strategies/build/workbox-strategies.prod.js	4.86 KB	4.86 KB	0%	1.19 KB
packages/workbox-streams/build/workbox-streams.prod.js	1.38 KB	1.38 KB	0%	678 B
packages/workbox-sw/build/workbox-sw.js	1.33 KB	1.33 KB	0%	741 B
packages/workbox-webpack-plugin/build/generate-sw.js	5.29 KB	5.29 KB	0%	1.84 KB
packages/workbox-webpack-plugin/build/index.js	349 B	349 B	0%	255 B
packages/workbox-webpack-plugin/build/inject-manifest.js	7.22 KB	7.22 KB	0%	2.48 KB
packages/workbox-window/build/workbox-window.dev.umd.js	30.42 KB	30.42 KB	0%	8.07 KB
packages/workbox-window/build/workbox-window.prod.umd.js	4.54 KB	4.54 KB	0%	1.81 KB

Workbox Aggregate Size Plugin

8.9KB gzip'ed (59% of limit)
22.5KB uncompressed

[bligny]

Hi @philipwalton

After upgrading to version 4.3, we are facing a new issue (that we did not have before) which is apparently related to the new cloning mechanism of replayed requests.

Here is the encountered error:

workbox-strategies.dev.js:829 Uncaught (in promise) no-response: 
The strategy could not generate a response for 'http://....:8380/api/v1/touring/tours/795/assignments/chanquez'. 
The underlying error is DataCloneError: Failed to execute 'add' on 'IDBObjectStore':
 async replayRequests() {
      let entry;

      while (entry = await this.shiftRequest()) {
        try {
    ...<omitted>... } could not be cloned..
    at NetworkOnly.makeRequest (http://localhost:8100/workbox-4.x/workbox-strategies.dev.js:829:15)
WorkboxError @ workbox-core.dev.js:382
makeRequest @ workbox-strategies.dev.js:829

Can this be a direct consequence of commit f165ca0 ?

For the sake of completeness, here is the request:

const httpOptions = {
  headers: new HttpHeaders(
    {'Content-Type': 'application/json',
     'Accept': 'application/json',
     'Authorization': 'Basic ******'),
    }),
    withCredentials: true,    
};
const targetUrl = `${touringUrl}/tours/${tourId}/assignments/${agentId}`;
return this.http.put<TourAssignment[]>(targetUrl, null, httpOptions).pipe(
  catchError(this.handleError)
);

and the applied WB strategy:

const queueUpload = new workbox.backgroundSync.Queue('tp-upload-v1', {
  maxRetentionTime: Infinity
});

const bgUpload = new workbox.backgroundSync.Plugin(queueUpload);

workbox.routing.registerRoute(
  new RegExp('.*/api/v1/touring/.*'),
  new workbox.strategies.NetworkOnly({
    fetchOptions: {
      credentials: 'include',
    },
    plugins: [bgUpload]
  }),
  'PUT'
);

[philipwalton]

For the sake of completeness, here is the request:

@bligny it's not clear to me where the code is running, as I'm not familiar with the APIs being used. Is that server-side code running in node?

and the applied WB strategy:

One thing that might be the problem is you're creating both a Queue instance and then you're creating a Plugin instance from that queue instance (which I don't think will work). The Plugin constructor takes the same arguments as the Queue constructor, and I don't think you should need both.

Try this (which just creates a Plugin instance):

const queuePlugin = new workbox.backgroundSync.Plugin('tp-upload-v1', {
  maxRetentionTime: Infinity
});

workbox.routing.registerRoute(
  new RegExp('.*/api/v1/touring/.*'),
  new workbox.strategies.NetworkOnly({
    fetchOptions: {
      credentials: 'include',
    },
    plugins: [queuePlugin]
  }),
  'PUT'
);

[bligny]

@philipwalton thx for spotting the mistake in my code.
The strange thing is that invoking the Plugin constructor with a Queue object, although incorrect, does not throw any error...
I'm pretty sure I've seen (and copy/pasted) this kind of construction somewhere, but I do not remember where...
Anyway, the strange behaviour is now explained. Thanks again

[bligny]

@philipwalton
FYI : I've just created #2042 in reaction to your comments

[philipwalton]

Thanks, that seems like a reasonable feature request.