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

sameOrigin param for matchCallback #2505

Merged
merged 1 commit into from
May 29, 2020
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
1 change: 1 addition & 0 deletions packages/workbox-core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export type PluginState = MapLikeObject;
*/
export interface RouteMatchCallbackOptions {
url: URL;
sameOrigin: boolean;
request: Request;
event?: ExtendableEvent;
}
Expand Down
35 changes: 12 additions & 23 deletions packages/workbox-routing/src/Router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {getFriendlyURL} from 'workbox-core/_private/getFriendlyURL.js';
import {Route} from './Route.js';
import {HTTPMethod} from './utils/constants.js';
import {normalizeHandler} from './utils/normalizeHandler.js';
import {Handler, HandlerObject, HandlerCallbackOptions} from './_types.js';
import {Handler, HandlerObject, HandlerCallbackOptions,
MatchCallbackOptions} from './_types.js';
import './_version.js';


Expand Down Expand Up @@ -171,7 +172,13 @@ class Router {
return;
}

const {params, route} = this.findMatchingRoute({url, request, event});
const sameOrigin = url.origin === location.origin;
const {params, route} = this.findMatchingRoute({
event,
request,
sameOrigin,
url,
});
let handler = route && route.handler;

const debugMessages = [];
Expand Down Expand Up @@ -264,30 +271,12 @@ class Router {
* They are populated if a matching route was found or `undefined`
* otherwise.
*/
findMatchingRoute({url, request, event}: {
url: URL;
request: Request;
event?: ExtendableEvent;
}): {route?: Route; params?: HandlerCallbackOptions['params']} {
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',
});
}

findMatchingRoute({url, sameOrigin, request, event}: MatchCallbackOptions):
{route?: Route; params?: HandlerCallbackOptions['params']} {
jeffposnick marked this conversation as resolved.
Show resolved Hide resolved
const routes = this._routes.get(request.method as HTTPMethod) || [];
for (const route of routes) {
let params;
const matchResult = route.match({url, request, event});
const matchResult = route.match({url, sameOrigin, request, event});
if (matchResult) {
// See https://github.com/GoogleChrome/workbox/issues/2079
params = matchResult;
Expand Down
16 changes: 9 additions & 7 deletions packages/workbox-routing/src/_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,11 @@ import './_version.js';
/**
* 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).
* client, the `event` object is supplied in addition to the `url`, `request`,
* and `sameOrigin` value. However, since the match callback can be invoked
* outside of a fetch event, matching logic should not assume the `event`
* object will always be available.
*
* If the match callback returns a truthy value, the matching route's
* [handler callback]{@link module:workbox-routing~handlerCallback} will be
* invoked immediately. If the value returned is a non-empty array or object,
Expand All @@ -47,10 +48,11 @@ import './_version.js';
* @callback ~matchCallback
* @param {Object} context
* @param {URL} context.url The request's URL.
* @param {Request} [context.request] The corresponding request,
* if available.
* @param {boolean} context.sameOrigin The result of comparing `url.origin`
* against the current origin.
* @param {Request} context.request The corresponding request.
jeffposnick marked this conversation as resolved.
Show resolved Hide resolved
* @param {FetchEvent} [context.event] The corresponding event that triggered
* the request, if available.
* the request, if available.
jeffposnick marked this conversation as resolved.
Show resolved Hide resolved
* @return {*} To signify a match, return a truthy value.
*
* @memberof module:workbox-routing
Expand Down
89 changes: 41 additions & 48 deletions test/workbox-routing/sw/test-Router.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -570,54 +570,48 @@ describe(`Router`, function() {
expect(handlerCallbackStub.firstCall.args[0].params).to.eql(returnValue);
});

it(`should not throw for router with no-routes set`, function() {
it(`should not throw for router with no-routes set`, async function() {
const router = new Router();

const request = new Request(location);
const event = new FetchEvent('fetch', {request});
router.handleRequest({request, event});
await router.handleRequest({request, event});
});
});

describe(`findMatchingRoute()`, function() {
it(`should throw in dev when not passed a URL`, async function() {
if (process.env.NODE_ENV === 'production') return this.skip();

it(`should set the matchCallback's sameOrigin to false when called with a cross-origin request`, async function() {
const router = new Router();
const url = new URL(location.href);
const request = new Request(url);
const event = new FetchEvent('fetch', {request});
const matchCallbackStub = sandbox.stub();
const route = new Route(
matchCallbackStub,
() => new Response(),
);
router.registerRoute(route);

await expectError(
() => router.findMatchingRoute({request, event}),
'incorrect-class',
(error) => {
expect(error.details).to.have.property('moduleName').that.eql('workbox-routing');
expect(error.details).to.have.property('className').that.eql('Router');
expect(error.details).to.have.property('funcName').that.eql('findMatchingRoute');
expect(error.details).to.have.property('paramName').that.eql('options.url');
});
});
const request = new Request('https://cross-origin.example.com');
await router.handleRequest({request});

it(`should throw in dev when not passed a request`, async function() {
if (process.env.NODE_ENV === 'production') return this.skip();
expect(matchCallbackStub.callCount).to.eql(1);
expect(matchCallbackStub.args[0][0].sameOrigin).to.be.false;
});

it(`should set the matchCallback's sameOrigin to true when called with a same-origin request`, async function() {
const router = new Router();
const url = new URL(location.href);
const request = new Request(url);
const event = new FetchEvent('fetch', {request});
const matchCallbackStub = sandbox.stub();
const route = new Route(
matchCallbackStub,
() => new Response(),
);
router.registerRoute(route);

await expectError(
() => router.findMatchingRoute({url, event}),
'incorrect-class',
(error) => {
expect(error.details).to.have.property('moduleName').that.eql('workbox-routing');
expect(error.details).to.have.property('className').that.eql('Router');
expect(error.details).to.have.property('funcName').that.eql('findMatchingRoute');
expect(error.details).to.have.property('paramName').that.eql('options.request');
});
const request = new Request(location.href);
await router.handleRequest({request});

expect(matchCallbackStub.callCount).to.eql(1);
expect(matchCallbackStub.args[0][0].sameOrigin).to.be.true;
});
});

describe(`findMatchingRoute()`, function() {
it(`should return the first matching route`, function() {
const router = new Router();

Expand All @@ -636,8 +630,9 @@ describe(`Router`, function() {
const url = new URL(location.href);
const request = new Request(url);
const event = new FetchEvent('fetch', {request});
const sameOrigin = true;

const {route} = router.findMatchingRoute({url, request, event});
const {route} = router.findMatchingRoute({url, sameOrigin, request, event});

expect(match1.callCount).to.equal(1);
expect(match2.callCount).to.equal(1);
Expand All @@ -660,18 +655,15 @@ describe(`Router`, function() {
const url = new URL(location.href);
const request = new Request(url);
const event = new FetchEvent('fetch', {request});
const sameOrigin = true;

const {route} = router.findMatchingRoute({url, request, event});
const {route} = router.findMatchingRoute({url, sameOrigin, request, event});

expect(match1.callCount).to.equal(1);
expect(match1.args[0][0].url).to.deep.equal(url);
expect(match1.args[0][0].request).to.equal(request);
expect(match1.args[0][0].event).to.equal(event);
expect(match1.args[0][0]).to.eql({url, sameOrigin, request, event});

expect(match2.callCount).to.equal(1);
expect(match2.args[0][0].url).to.deep.equal(url);
expect(match2.args[0][0].request).to.equal(request);
expect(match2.args[0][0].event).to.equal(event);
expect(match2.args[0][0]).to.eql({url, sameOrigin, request, event});

expect(route).to.equal(route2);
});
Expand All @@ -692,28 +684,29 @@ describe(`Router`, function() {
const url = new URL(location.href);
const request = new Request(url);
const event = new FetchEvent('fetch', {request});
const sameOrigin = true;

const result1 = router.findMatchingRoute({url, request, event});
const result1 = router.findMatchingRoute({url, sameOrigin, request, event});
expect(result1.route).to.equal(route);
expect(result1.params).to.equal(undefined);

const result2 = router.findMatchingRoute({url, request, event});
const result2 = router.findMatchingRoute({url, sameOrigin, request, event});
expect(result2.route).to.equal(route);
expect(result2.params).to.equal('truthy');

const result3 = router.findMatchingRoute({url, request, event});
const result3 = router.findMatchingRoute({url, sameOrigin, request, event});
expect(result3.route).to.equal(route);
expect(result3.params).to.deep.equal([1, 2, 3]);

const result4 = router.findMatchingRoute({url, request, event});
const result4 = router.findMatchingRoute({url, sameOrigin, request, event});
expect(result4.route).to.equal(route);
expect(result4.params).to.equal(undefined);

const result5 = router.findMatchingRoute({url, request, event});
const result5 = router.findMatchingRoute({url, sameOrigin, request, event});
expect(result5.route).to.equal(route);
expect(result5.params).to.deep.equal({a: 1});

const result6 = router.findMatchingRoute({url, request, event});
const result6 = router.findMatchingRoute({url, sameOrigin, request, event});
expect(result6.route).to.equal(route);
expect(result6.params).to.equal(undefined);
});
Expand Down