Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Background sync queue callbacks #666

Merged
merged 10 commits into from
Jul 11, 2017
33 changes: 33 additions & 0 deletions lib/deprecate.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
Copyright 2017 Google Inc. All Rights Reserved.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

/**
* Warns users that an old object method is deprecated in favor of a new method
* and aliases the old name to the new name.
* @param {Object} obj The object containing the methods.
* @param {string} base The base project/object to identify the method names.
* @param {string} oldMethod The method to deprecate.
* @param {string} newMethod The new method replacing the deprecated method.
*/
export default (obj, base, oldMethod, newMethod) => {
/* eslint-disable no-console */
console.warn(`In ${base}: ` +
`${oldMethod} is deprecated, use ${newMethod} instead`);
/* eslint-enable no-console */

if (obj[oldMethod] && !obj[newMethod]) {
obj[newMethod] = obj[oldMethod];
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import Queue from './background-sync-queue';
* failed requests.</caption>
* let bgQueue = new workbox.backgroundSync.QueuePlugin({
* callbacks: {
* onResponse: async(hash, res) => {
* replayDidSucceed: async(hash, res) => {
* self.registration.showNotification('Background sync demo', {
* body: 'Product has been purchased.',
* icon: '/images/shop-icon-384.png',
* });
* },
* onRetryFailure: (hash) => {},
* replayDidFail: (hash) => {},
* requestWillEnqueue: (reqData) => {},
* requestWillDequeue: (reqData) => {},
* },
* });
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ class Queue {
* @param {Object} [input]
* @param {Number} [input.maxRetentionTime = 5 days] Time for which a queued
* request will live in the queue(irespective of failed/success of replay).
* @param {Object} [input.callbacks] Callbacks for successfull/ failed
* replay of a request.
* @param {Object} [input.callbacks] Callbacks for successfull/failed
* replay of a request as well as modifying before enqueue/dequeue-ing.
* @param {string} [input.queueName] Queue name inside db in which
* requests will be queued.
* @param {BroadcastChannel=} [input.broadcastChannel] BroadcastChannel
Expand Down Expand Up @@ -66,9 +66,12 @@ class Queue {
queueName,
idbQDb: new IDBHelper(this._dbName, 1, 'QueueStore'),
broadcastChannel,
callbacks,
});
this._requestManager = new RequestManager({
callbacks,
queue: this._queue,
});
this._requestManager = new RequestManager({callbacks,
queue: this._queue});

this.cleanupQueue();
}
Expand Down
21 changes: 15 additions & 6 deletions packages/workbox-background-sync/src/lib/request-manager.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import deprecate from '../../../../lib/deprecate';
import {putResponse} from './response-manager';
import {getFetchableRequest} from './queue-utils';
import {tagNamePrefix, replayAllQueuesTag} from './constants';
Expand All @@ -18,8 +19,15 @@ class RequestManager {
*
* @private
*/
constructor({callbacks, queue}) {
this._globalCallbacks = callbacks || {};
constructor({callbacks, queue} = {}) {
callbacks = callbacks || {};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

deprecate(callbacks, base, 'onRetryFailure', 'replayDidFail');

this._globalCallbacks = callbacks;
this._queue = queue;
this.attachSyncHandler();
}
Expand Down Expand Up @@ -70,8 +78,8 @@ class RequestManager {
response: response.clone(),
idbQDb: this._queue.idbQDb,
});
if (this._globalCallbacks.onResponse)
this._globalCallbacks.onResponse(hash, response);
if (this._globalCallbacks.replayDidSucceed)
this._globalCallbacks.replayDidSucceed(hash, response);
}
} catch(err) {
return Promise.reject(err);
Expand All @@ -94,8 +102,9 @@ class RequestManager {
try {
await this.replayRequest(hash);
} catch (err) {
if(this._globalCallbacks.onRetryFailure)
this._globalCallbacks.onRetryFailure(hash, err);
if (this._globalCallbacks.replayDidFail) {
this._globalCallbacks.replayDidFail(hash, err);
}
failedItems.push(err);
}
}
Expand Down
40 changes: 30 additions & 10 deletions packages/workbox-background-sync/src/lib/request-queue.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ class RequestQueue {
queueName = defaultQueueName + '_' + _queueCounter++,
idbQDb,
broadcastChannel,
callbacks,
}) {
this._isQueueNameAddedToAllQueue = false;
this._queueName = queueName;
this._config = config;
this._idbQDb = idbQDb;
this._broadcastChannel = broadcastChannel;
this._globalCallbacks = callbacks || {};
this._queue = [];
this.initQueue();
}
Expand Down Expand Up @@ -86,6 +88,7 @@ class RequestQueue {
* preferably when network comes back
*
* @param {Request} request request object to be queued by this
* @return {Promise}
*
* @memberOf Queue
* @private
Expand All @@ -94,17 +97,23 @@ class RequestQueue {
isInstance({request}, Request);

const hash = `${request.url}!${Date.now()}!${_requestCounter++}`;
const queuableRequest =
await getQueueableRequest({
request,
config: this._config,
});
const reqData = await getQueueableRequest({
request,
config: this._config,
});

// Apply the `requestWillEnqueue` callback so plugins can modify the
// request data before it's stored in IndexedDB.
if (this._globalCallbacks.requestWillEnqueue) {
this._globalCallbacks.requestWillEnqueue(reqData);
}

try{
this._queue.push(hash);

// add to queue
this.saveQueue();
this._idbQDb.put(hash, queuableRequest);
this._idbQDb.put(hash, reqData);
await this.addQueueNameToAllQueues();
// register sync
self.registration &&
Expand All @@ -117,31 +126,42 @@ class RequestQueue {
id: hash,
url: request.url,
});
} catch(e) {

return hash;
} catch (err) {
// broadcast the failure of request added to the queue
broadcastMessage({
broadcastChannel: this._broadcastChannel,
type: broadcastMessageFailedType,
id: hash,
url: request.url,
});

return err;
}
}

/**
* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{Promise<Request>}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad it is indeed

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

* @memberOf Queue
* @private
*/
async getRequestFromQueue({hash}) {
isType({hash}, 'string');

if(this._queue.includes(hash)) {
const req = await this._idbQDb.get(hash);
return req;
const reqData = await this._idbQDb.get(hash);

// Apply the `requestWillDequeue` callback so plugins can modify the
// stored data before it's converted back into a request to be replayed.
if (this._globalCallbacks.requestWillDequeue) {
this._globalCallbacks.requestWillDequeue(reqData);
}

return reqData;
}
}

Expand Down
49 changes: 36 additions & 13 deletions packages/workbox-background-sync/test/browser/request-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,11 @@
*/

/* eslint-env mocha, browser */
/* global chai, workbox */
/* global chai, sinon, workbox */

'use strict';
describe('request-manager test', () => {
let responseAchieved = 0;
const callbacks = {
onResponse: function() {
responseAchieved ++;
},
};

const callbacks = {};
let queue;
let reqManager;

Expand Down Expand Up @@ -56,13 +50,42 @@ describe('request-manager test', () => {
});

it('check replay', async function() {
const backgroundSyncQueue
= new workbox.backgroundSync.test.BackgroundSyncQueue({
callbacks,
});
sinon.spy(self, 'fetch');

callbacks.replayDidSucceed = sinon.spy();
callbacks.replayDidFail = sinon.spy();

const backgroundSyncQueue =
new workbox.backgroundSync.test.BackgroundSyncQueue({callbacks});

await backgroundSyncQueue.pushIntoQueue({request: new Request('/__echo/counter')});
await backgroundSyncQueue.pushIntoQueue({request: new Request('/__echo/counter')});
await backgroundSyncQueue._requestManager.replayRequests();
chai.assert.equal(responseAchieved, 2);

// Asset replayDidSucceed callback was called with the correct arguments.
chai.assert.equal(callbacks.replayDidSucceed.callCount, 2);
chai.assert(callbacks.replayDidSucceed.alwaysCalledWith(
sinon.match.string, sinon.match.instanceOf(Response)));

// Assert fetch was called for each replayed request.
chai.assert(self.fetch.calledTwice);

await backgroundSyncQueue.pushIntoQueue({request: new Request('/__test/404')});
try {
await backgroundSyncQueue._requestManager.replayRequests();
} catch (err) {
// Error is expected due to 404 response.
}

// Asset replayDidFail callback was called with the correct arguments.
chai.assert.equal(callbacks.replayDidSucceed.callCount, 2);
chai.assert.equal(callbacks.replayDidFail.callCount, 1);
chai.assert(callbacks.replayDidFail.alwaysCalledWith(
sinon.match.string, sinon.match.instanceOf(Response)));

delete callbacks.replayDidSucceed;
delete callbacks.replayDidFail;

self.fetch.restore();
});
});
54 changes: 41 additions & 13 deletions packages/workbox-background-sync/test/browser/request-queue.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,23 @@
*/

/* eslint-env mocha, browser */
/* global chai, workbox */
/* global chai, sinon, workbox */

'use strict';

describe('request-queue tests', () => {
const QUEUE_NAME = 'QUEUE_NAME';
const MAX_AGE = 6;

const callbacks = {};
const idbHelper = new workbox.backgroundSync.test.IdbHelper(
'bgQueueSyncDB', 1, 'QueueStore');
let queue =
new workbox.backgroundSync.test.RequestQueue({
idbQDb: idbHelper,
config: {maxAge: MAX_AGE},
queueName: QUEUE_NAME,
});
const queue = new workbox.backgroundSync.test.RequestQueue({
idbQDb: idbHelper,
config: {maxAge: MAX_AGE},
queueName: QUEUE_NAME,
callbacks,
});

it('queue object should exist', () => {
chai.assert.isObject(queue);
Expand All @@ -45,12 +47,38 @@ describe('request-queue tests', () => {
queue._config.maxAge, workbox.backgroundSync.test.Constants.maxAge);
});

it('pushRequest is working', () => {
let queueLength = queue._queue.length;
return queue.push({request: new Request('http://lipsum.com/generate')})
.then(() => {
chai.assert.equal(queue._queue.length, queueLength + 1);
});
it('push is working', async () => {
callbacks.requestWillEnqueue = sinon.spy();

const queueLength = queue._queue.length;
const hash = await queue.push({
request: new Request('http://lipsum.com/generate'),
});

chai.assert.isString(hash);
chai.assert.equal(queue._queue.length, queueLength + 1);

chai.assert(callbacks.requestWillEnqueue.calledOnce);
chai.assert(callbacks.requestWillEnqueue.calledWith(
sinon.match.has('request')));

delete callbacks.requestWillEnqueue;
});

it('getRequestFromQueue is working', async () => {
callbacks.requestWillDequeue = sinon.spy();

const hash = await queue.push({
request: new Request('http://lipsum.com/generate'),
});

const reqData = await queue.getRequestFromQueue({hash});

chai.assert.hasAllKeys(reqData, ['request', 'config', 'metadata']);
chai.assert(callbacks.requestWillDequeue.calledOnce);
chai.assert(callbacks.requestWillDequeue.calledWith(reqData));

delete callbacks.requestWillDequeue;
});

it('default config is correct', () => {
Expand Down