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

Decouple Router~handleRequest from events #1682

Merged
merged 2 commits into from
Oct 8, 2018
Merged
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
2 changes: 1 addition & 1 deletion packages/workbox-build/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions packages/workbox-google-analytics/initialize.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,11 @@ const initialize = (options = {}) => {
router.registerRoute(route);
}

self.addEventListener('fetch', (evt) => {
const responsePromise = router.handleRequest(evt);
self.addEventListener('fetch', (event) => {
const {request} = event;
const responsePromise = router.handleRequest({request, event});
if (responsePromise) {
evt.respondWith(responsePromise);
event.respondWith(responsePromise);
}
});
};
Expand Down
8 changes: 4 additions & 4 deletions packages/workbox-routing/NavigationRoute.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class NavigationRoute extends Route {
});
}

super((...args) => this._match(...args), handler);
super((options) => this._match(options), handler);

this._whitelist = whitelist;
this._blacklist = blacklist;
Expand All @@ -72,14 +72,14 @@ class NavigationRoute extends Route {
* Routes match handler.
*
* @param {Object} options
* @param {FetchEvent} options.event
* @param {URL} options.url
* @param {Request} options.request
* @return {boolean}
*
* @private
*/
_match({event, url}) {
if (event.request.mode !== 'navigate') {
_match({url, request}) {
if (request.mode !== 'navigate') {
return false;
}

Expand Down
97 changes: 53 additions & 44 deletions packages/workbox-routing/Router.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,26 @@ class Router {
* Apply the routing rules to a FetchEvent object to get a Response from an
* appropriate Route's handler.
*
* @param {FetchEvent} event The event from a service worker's 'fetch' event
* listener.
* @param {Object} options
* @param {Request} options.request The request to handle (this is usually
* from a fetch event, but it does not have to be).
* @param {FetchEvent} [options.event] The event that triggered the request,
* if applicable.
* @return {Promise<Response>|undefined} A promise is returned if a
* registered route can handle the FetchEvent's request. If there is no
* matching route and there's no `defaultHandler`, `undefined` is returned.
* registered route can handle the request. If there is no matching
* route and there's no `defaultHandler`, `undefined` is returned.
*/
handleRequest(event) {
handleRequest({request, event}) {
if (process.env.NODE_ENV !== 'production') {
assert.isInstance(event, FetchEvent, {
assert.isInstance(request, Request, {
moduleName: 'workbox-routing',
className: 'Router',
funcName: 'handleRequest',
paramName: 'event',
paramName: 'options.request',
});
}

const url = new URL(event.request.url);
const url = new URL(request.url, location);
if (!url.protocol.startsWith('http')) {
if (process.env.NODE_ENV !== 'production') {
logger.debug(
Expand All @@ -70,15 +73,10 @@ class Router {
return;
}

let route = null;
let handler = null;
let params = null;
let debugMessages = [];
let {params, route} = this.findMatchingRoute({url, request, event});
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like all these lets can be changed to const after your refactoring.

Copy link
Member Author

Choose a reason for hiding this comment

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

route gets reassigned and params doesn't, but since one of them does, in order to use destructuring I had to do both as let...

let handler = route && route.handler;

const result = this._findHandlerAndParams(event, url);
handler = result.handler;
params = result.params;
route = result.route;
let debugMessages = [];
if (process.env.NODE_ENV !== 'production') {
if (handler) {
debugMessages.push([
Expand Down Expand Up @@ -130,7 +128,7 @@ class Router {
// The Request and Response objects contains a great deal of information,
// hide it under a group in case developers want to see it.
logger.groupCollapsed(`View request details here.`);
logger.unprefixed.log(event.request);
logger.unprefixed.log(request);
logger.groupEnd();

logger.groupEnd();
Expand All @@ -140,7 +138,7 @@ class Router {
// error. It should still callback to the catch handler.
let responsePromise;
try {
responsePromise = handler.handle({url, event, params});
responsePromise = handler.handle({url, request, event, params});
} catch (err) {
responsePromise = Promise.reject(err);
}
Expand All @@ -164,43 +162,54 @@ class Router {
}

/**
* Checks the incoming `event.request` against the registered routes, and if
* there's a match, returns the corresponding handler along with any params
* generated by the match.
* Checks a request and URL (and optionally an event) against the list of
* registered routes, and if there's a match, returns the corresponding
* route along with any params generated by the match.
*
* @param {FetchEvent} event
* @param {URL} url
* @return {Object} Returns an object with `handler` and `params` properties.
* They are populated if a matching route was found or `undefined` otherwise.
*
* @private
* @param {Object} options
* @param {URL} options.url
* @param {Request} options.request The request to match.
* @param {FetchEvent} [options.event] The corresponding event (unless N/A).
* @return {Object} An object with `route` and `params` properties.
* They are populated if a matching route was found or `undefined`
* otherwise.
*/
_findHandlerAndParams(event, url) {
const routes = this._routes.get(event.request.method) || [];
findMatchingRoute({url, request, event}) {
if (process.env.NODE_ENV !== 'production') {
assert.isInstance(url, URL, {
moduleName: 'workbox-routing',
className: 'Router',
funcName: 'findMatchingRoute',
paramName: 'options.url',
});
assert.isInstance(request, Request, {
moduleName: 'workbox-routing',
className: 'Router',
funcName: 'findMatchingRoute',
paramName: 'options.request',
});
}

const routes = this._routes.get(request.method) || [];
for (const route of routes) {
let matchResult = route.match({url, event});
let params;
let matchResult = route.match({url, request, event});
if (matchResult) {
if (Array.isArray(matchResult) && matchResult.length === 0) {
if (Array.isArray(matchResult) && matchResult.length > 0) {
// Instead of passing an empty array in as params, use undefined.
matchResult = undefined;
params = matchResult;
} else if ((matchResult.constructor === Object &&
Object.keys(matchResult).length === 0) || matchResult === true) {
Object.keys(matchResult).length > 0)) {
// Instead of passing an empty object in as params, use undefined.
matchResult = undefined;
params = matchResult;
}

// Break out of the loop and return the appropriate values as soon as
// we have a match.
return {
route,
params: matchResult,
handler: route.handler,
};
// Return early if have a match.
return {route, params};
}
}

// If we didn't have a match, then return undefined values.
return {handler: undefined, params: undefined};
// If no match was found above, return and empty object.
return {};
}

/**
Expand Down
3 changes: 2 additions & 1 deletion packages/workbox-routing/_default.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,8 @@ const router = new DefaultRouter();
// By default, register a fetch event listener that will respond to a request
// only if there's a matching route.
self.addEventListener('fetch', (event) => {
const responsePromise = router.handleRequest(event);
const request = event.request;
const responsePromise = router.handleRequest({request, event});
if (responsePromise) {
event.respondWith(responsePromise);
}
Expand Down
54 changes: 29 additions & 25 deletions packages/workbox-routing/_types.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -9,45 +9,49 @@
import './_version.mjs';

/**
* The "match" callback is used to determine if a `Route` should handle a
* service worker's `fetch` event. Returning a truthy value means
* this `Route` will handle and respond to the event.
*
* Return `null` if this Route shouldn't match against the `fetch` event.
*
* Any truthy value returned by this callback will be passed to the
* Route's
* [handler callback]{@link workbox.routing.Route~handlerCallback}.
* The "match" callback is used to determine if a `Route` should apply for a
* particular URL. When matching occurs in response to a fetch event from the
* client, the `event` and `request` objects are supplied in addition to the
* URL. However, since the match callback can be invoked outside of a fetch
* event, matching logic should not assume the `event` or `request` objects
* will always be available (unlike URL, which is always available).
* If the match callback returns a truthy value, the matching route's
* [handler callback]{@link workbox.routing.Route~handlerCallback} will be
* invoked immediately. If the value returned is a non-empty array or object,
* that value will be set on the handler's `context.params` argument.
*
* @callback Route~matchCallback
* @param {Object} context
* @param {URL} context.url The request's URL.
* @param {FetchEvent} context.event The service worker`s `fetch`
* event.
* @return {Object|null} To signify a match, return anything other than `null`.
* Return `null` if the route shouldn't match.
* [handler]{@link workbox.routing.Route~handlerCallback}.
* @param {FetchEvent} [context.request] The corresponding request,
* if available.
* @param {FetchEvent} [context.event] The corresponding event that triggered
* the request, if available.
* @return {*} To signify a match, return a truthy value.
*
* @memberof workbox.routing
*/

/**
* The "handler" callback is called when a service worker's `fetch` event
* has been matched by a `Route`. This callback should return a Promise that
* resolves with a `Response`.
* The "handler" callback is invoked whenever a `Router` matches a URL to a
* `Route` via its [match]{@link workbox.routing.Route~handlerCallback}
* callback. This callback should return a Promise that resolves with a
* `Response`.
*
* If a value is returned by the
* If a non-empty array or object is returned by the
* [match callback]{@link workbox.routing.Route~matchCallback} it
* will be passed in as the `context.params` argument.
* will be passed in as the handler's `context.params` argument.
*
* @callback Route~handlerCallback
* @param {Object} context
* @param {URL} context.url The request's URL.
* @param {FetchEvent} context.event The service worker's `fetch`
* event.
* @param {Object} [context.params] Parameters returned by the Route's
* [match callback]{@link workbox.routing.Route~matchCallback} function.
* This will be undefined if nothing was returned.
* @param {URL} context.url The URL that matched.
* @param {FetchEvent} [context.request] The corresponding request,
* if available.
* @param {FetchEvent} [context.event] The corresponding event that triggered
* the request, if available.
* @param {Object} [context.params] Array or Object parameters returned by the
* Route's [match callback]{@link workbox.routing.Route~matchCallback}.
* This will be undefined if an empty array or object were returned.
* @return {Promise<Response>} The response that will fulfill the request.
*
* @memberof workbox.routing
Expand Down
17 changes: 4 additions & 13 deletions packages/workbox-strategies/CacheFirst.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -56,23 +56,14 @@ class CacheFirst {
* [Workbox Router]{@link workbox.routing.Router}.
*
* @param {Object} options
* @param {FetchEvent} options.event The fetch event to run this strategy
* against.
* @param {Request} options.request The request to run this strategy for.
* @param {Event} [options.event] The event that triggered the request.
* @return {Promise<Response>}
*/
async handle({event}) {
if (process.env.NODE_ENV !== 'production') {
assert.isInstance(event, FetchEvent, {
moduleName: 'workbox-strategies',
className: 'CacheFirst',
funcName: 'handle',
paramName: 'event',
});
}

async handle({event, request}) {
return this.makeRequest({
event,
request: event.request,
request: request || event.request,
});
}

Expand Down
17 changes: 4 additions & 13 deletions packages/workbox-strategies/CacheOnly.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,14 @@ class CacheOnly {
* [Workbox Router]{@link workbox.routing.Router}.
*
* @param {Object} options
* @param {FetchEvent} options.event The fetch event to run this strategy
* against.
* @param {Request} options.request The request to run this strategy for.
* @param {Event} [options.event] The event that triggered the request.
* @return {Promise<Response>}
*/
async handle({event}) {
if (process.env.NODE_ENV !== 'production') {
assert.isInstance(event, FetchEvent, {
moduleName: 'workbox-strategies',
className: 'CacheOnly',
funcName: 'handle',
paramName: 'event',
});
}

async handle({event, request}) {
return this.makeRequest({
event,
request: event.request,
request: request || event.request,
});
}

Expand Down
17 changes: 4 additions & 13 deletions packages/workbox-strategies/NetworkFirst.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -87,23 +87,14 @@ class NetworkFirst {
* [Workbox Router]{@link workbox.routing.Router}.
*
* @param {Object} options
* @param {FetchEvent} options.event The fetch event to run this strategy
* against.
* @param {Request} options.request The request to run this strategy for.
* @param {Event} [options.event] The event that triggered the request.
* @return {Promise<Response>}
*/
async handle({event}) {
if (process.env.NODE_ENV !== 'production') {
assert.isInstance(event, FetchEvent, {
moduleName: 'workbox-strategies',
className: 'NetworkFirst',
funcName: 'handle',
paramName: 'event',
});
}

async handle({event, request}) {
return this.makeRequest({
event,
request: event.request,
request: request || event.request,
});
}

Expand Down
17 changes: 4 additions & 13 deletions packages/workbox-strategies/NetworkOnly.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -51,23 +51,14 @@ class NetworkOnly {
* [Workbox Router]{@link workbox.routing.Router}.
*
* @param {Object} options
* @param {FetchEvent} options.event The fetch event to run this strategy
* against.
* @param {Request} options.request The request to run this strategy for.
* @param {Event} [options.event] The event that triggered the request.
* @return {Promise<Response>}
*/
async handle({event}) {
if (process.env.NODE_ENV !== 'production') {
assert.isInstance(event, FetchEvent, {
moduleName: 'workbox-strategies',
className: 'NetworkOnly',
funcName: 'handle',
paramName: 'event',
});
}

async handle({event, request}) {
return this.makeRequest({
event,
request: event.request,
request: request || event.request,
});
}

Expand Down
Loading