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

Makes it easier to use custom Routes within WorkboxSW. #639

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions lib/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,36 @@ export function isArrayOfClass(object, expectedClass) {
}
}

export function isInterface(object, expectedProperties) {
const parameter = Object.keys(object).pop();
const message = `The '${parameter}' parameter should be an object that has
'${expectedProperties}' properties.`;

for (let property of expectedProperties) {
if (!(property in object[parameter])) {
throwError(message);

Choose a reason for hiding this comment

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

Would it be worth adding the missing parameter to the message?

}
}
}

export function isArrayOfInterface(object, expectedProperties) {
const parameter = Object.keys(object).pop();
const message = `The '${parameter}' parameter should be an array containing
objects that each have '${expectedProperties}' properties.`;

if (!Array.isArray(object[parameter])) {
throwError(message);
}

for (let item of object[parameter]) {
for (let property of expectedProperties) {
if (!(property in item)) {
throwError(message);

Choose a reason for hiding this comment

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

same as above, the message should be updated to handle the missing property

}
}
}
}

export function isValue(object, expectedValue) {
const parameter = Object.keys(object).pop();
const actualValue = object[parameter];
Expand Down
16 changes: 10 additions & 6 deletions packages/workbox-routing/src/lib/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@
limitations under the License.
*/

import Route from './route';
import {isArrayOfClass, isInstance} from '../../../../lib/assert';
import {isArrayOfInterface, isInterface} from '../../../../lib/assert';
import logHelper from '../../../../lib/log-helper.js';
import normalizeHandler from './normalize-handler';

const ROUTE_INTERFACE = ['match', 'handler', 'method'];

Choose a reason for hiding this comment

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

I'm not a huge fan of having this manually defined and it being defined away from the class it represents.


/**
* The Router takes one or more [Routes]{@link Route} and registers a [`fetch`
* event listener](https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent)
Expand Down Expand Up @@ -220,7 +221,10 @@ class Router {
* routes to register.
*/
registerRoutes({routes} = {}) {
isArrayOfClass({routes}, Route);
// Rather than explicitly checking for Route subclasses, check to ensure
// that the object exposes the interface we need.
// See https://github.com/GoogleChrome/workbox/issues/385
isArrayOfInterface({routes}, ROUTE_INTERFACE);

for (let route of routes) {
if (!this._routes.has(route.method)) {
Expand All @@ -244,7 +248,7 @@ class Router {
* @param {module:workbox-routing.Route} input.route The route to register.
*/
registerRoute({route} = {}) {
isInstance({route}, Route);
isInterface({route}, ROUTE_INTERFACE);

this.registerRoutes({routes: [route]});
}
Expand All @@ -265,7 +269,7 @@ class Router {
* routes to unregister.
*/
unregisterRoutes({routes} = {}) {
isArrayOfClass({routes}, Route);
isArrayOfInterface({routes}, ROUTE_INTERFACE);

for (let route of routes) {
if (!this._routes.has(route.method)) {
Expand Down Expand Up @@ -305,7 +309,7 @@ class Router {
* @param {module:workbox-routing.Route} input.route The route to unregister.
*/
unregisterRoute({route} = {}) {
isInstance({route}, Route);
isInterface({route}, ROUTE_INTERFACE);

this.unregisterRoutes({routes: [route]});
}
Expand Down
43 changes: 43 additions & 0 deletions packages/workbox-routing/test/sw/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,47 @@ describe('Test of the Router class', function() {
expect(router._routes.get('GET')).to.have.members([getRoute1]);
expect(router._routes.get('PUT')).to.have.members([putRoute1]);
});

it(`should fail an assertion when registerRoute() is called with an invalid route`, function() {
const router = new workbox.routing.Router();
const route = {};
try {
router.registerRoute({route});
throw new Error();
} catch(error) {
expect(error.name).to.eql('assertion-failed',
`The expected assertion-failed error wasn't thrown.`);
}
});

it(`should fail an assertion when registerRoutes() is called with one invalid route`, function() {
const router = new workbox.routing.Router();
const routes = [
new workbox.routing.Route({match: MATCH, handler: HANDLER}),
{},
];

try {
router.registerRoutes({routes});
throw new Error();
} catch(error) {
expect(error.name).to.eql('assertion-failed',
`The expected assertion-failed error wasn't thrown.`);
}
});

it(`should succeed when registerRoute() is called with something matching the Route interface`, function() {
const router = new workbox.routing.Router();
const route = {method: 'GET', match: MATCH, handler: HANDLER};
router.registerRoute({route});
});

it(`should succeed when registerRoutes() is called with multiple things matching the Route interface`, function() {
const router = new workbox.routing.Router();
const routes = [
new workbox.routing.Route({match: MATCH, handler: HANDLER}),
{method: 'GET', match: MATCH, handler: HANDLER},
];
router.registerRoutes({routes});
});
});
8 changes: 5 additions & 3 deletions packages/workbox-sw/src/lib/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,12 @@ class Router extends SWRoutingRouter {
* @param {function|module:workbox-runtime-caching.Handler} handler The
* handler to use to provide a response if the route matches. The handler
* argument is ignored if you pass in a Route object, otherwise it's required.
* @param {String} [method] Only match requests that use this HTTP method.
* Defaults to `'GET'`.
* @return {module:workbox-routing.Route} The Route object that was
* registered.
*/
registerRoute(capture, handler) {
registerRoute(capture, handler, method = 'GET') {
if (typeof handler === 'function') {
handler = {
handle: handler,
Expand All @@ -97,9 +99,9 @@ class Router extends SWRoutingRouter {
if (capture.length === 0) {
throw ErrorFactory.createError('empty-express-string');
}
route = new ExpressRoute({path: capture, handler});
route = new ExpressRoute({path: capture, handler, method});

Choose a reason for hiding this comment

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

why did you add the method?

} else if (capture instanceof RegExp) {
route = new RegExpRoute({regExp: capture, handler});
route = new RegExpRoute({regExp: capture, handler, method});
} else if (capture instanceof Route) {
route = capture;
} else {
Expand Down
18 changes: 18 additions & 0 deletions packages/workbox-sw/test/sw/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,22 @@ describe('Test workboxSW.router', function() {
expect(thrownError).to.exist;
expect(thrownError.name).to.equal('navigation-route-url-string');
});

it(`should throw when the method passed in to registerRoute() isn't valid`, function() {
const workboxSW = new WorkboxSW();
try {
workboxSW.router.registerRoute(/123/, () => {}, 'INVALID_METHOD');
throw new Error();
} catch(error) {
expect(error.name).to.eql('assertion-failed',
`The expected assertion-failed error wasn't thrown.`);
}
});

it(`should use the valid method passed in to registerRoute()`, function() {
const workboxSW = new WorkboxSW();
const method = 'POST';
const route =workboxSW.router.registerRoute(/123/, () => {}, method);
expect(route.method).to.eql(method);
});
});