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

Route/Handler TypeScript normalization #2548

Merged
merged 8 commits into from
Jun 30, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
69 changes: 46 additions & 23 deletions packages/workbox-core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ export type PluginState = MapLikeObject;
* Options passed to a `RouteMatchCallback` function.
*/
export interface RouteMatchCallbackOptions {
url: URL;
sameOrigin: boolean;
event: ExtendableEvent;
request: Request;
event?: ExtendableEvent;
sameOrigin: boolean;
url: URL;
}

/**
Expand All @@ -48,12 +48,23 @@ export interface RouteMatchCallback {
* Options passed to a `RouteHandlerCallback` function.
*/
export interface RouteHandlerCallbackOptions {
request: Request | string;
url?: URL;
event: ExtendableEvent;
request: Request;
url: URL;
params?: string[] | MapLikeObject;
}

/**
* Options passed to a `ManualHandlerCallback` function.
*/
export interface ManualHandlerCallbackOptions {
philipwalton marked this conversation as resolved.
Show resolved Hide resolved
request: Request | string;
event?: ExtendableEvent;
Copy link
Member

Choose a reason for hiding this comment

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

I believe we want to make this required in v6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right. We had talked about how using strategies manually outside of an event handler wasn't a great idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One follow-up from that is that I need to change a lot of the parameter signatures in the PrecacheController class to mark the ExtendableEvent as being required. (That's probably a good thing, since using PrecacheController outside of an event sounds like a recipe for disaster.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change also filters through to the MatchWrapperOptions and PutWrapperOptions interfaces, which now need to mark the event as required.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if you want to do this as part of this PR or a separate on, but there are many cases in our code where we've had to make an event optional in a type definition due to us not making it required in handlers (e.g. here). I did a in VSCode for event?:, and lots of cases come up (and there are probably others where the name "event" isn't used).

Also, there are cases where we're conditionally checking whether an event was passed (e.g. here), and it probably makes sense to remove those now.

How do you want to handle these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can get those updated in this PR as well.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I went through and updated the corresponding jsdoc comments for a few that where missed (it was easier than going back and forth).

}

export type HandlerCallbackOptions =
philipwalton marked this conversation as resolved.
Show resolved Hide resolved
RouteHandlerCallbackOptions | ManualHandlerCallbackOptions;

/**
* The "handler" callback is invoked whenever a `Router` matches a URL/Request
* to a `Route` via its `RouteMatchCallback`. This handler callback should
Expand All @@ -66,6 +77,18 @@ export interface RouteHandlerCallback {
(options: RouteHandlerCallbackOptions): Promise<Response>;
}

/**
* The "handler" callback is invoked whenever a `Router` matches a URL/Request
* to a `Route` via its `RouteMatchCallback`. This handler callback should
* return a `Promise` that resolves with a `Response`.
*
* If a non-empty array or object is returned by the `RouteMatchCallback` it
* will be passed in as this handler's `options.params` argument.
*/
export interface HandlerCallback {
philipwalton marked this conversation as resolved.
Show resolved Hide resolved
(options: HandlerCallbackOptions): Promise<Response>;
}

/**
* An object with a `handle` method of type `RouteHandlerCallback`.
*
Expand Down Expand Up @@ -95,10 +118,10 @@ export interface HandlerWillStartCallback {

export interface CacheDidUpdateCallbackParam {
cacheName: string;
oldResponse?: Response | null;
newResponse: Response;
request: Request;
event?: ExtendableEvent;
oldResponse?: Response | null;
state?: PluginState;
}

Expand All @@ -107,10 +130,10 @@ export interface CacheDidUpdateCallback {
}

export interface CacheKeyWillBeUsedCallbackParam {
request: Request;
mode: string;
params?: any;
request: Request;
event?: ExtendableEvent;
params?: any;
state?: PluginState;
}

Expand All @@ -119,8 +142,8 @@ export interface CacheKeyWillBeUsedCallback {
}

export interface CacheWillUpdateCallbackParam {
response: Response;
request: Request;
response: Response;
event?: ExtendableEvent;
state?: PluginState;
}
Expand All @@ -132,9 +155,9 @@ export interface CacheWillUpdateCallback {
export interface CachedResponseWillBeUsedCallbackParam {
cacheName: string;
request: Request;
matchOptions?: CacheQueryOptions;
cachedResponse?: Response;
event?: ExtendableEvent;
matchOptions?: CacheQueryOptions;
state?: PluginState;
}

Expand All @@ -143,8 +166,8 @@ export interface CachedResponseWillBeUsedCallback {
}

export interface FetchDidFailCallbackParam {
originalRequest: Request;
error: Error;
originalRequest: Request;
request: Request;
event?: ExtendableEvent;
state?: PluginState;
Expand Down Expand Up @@ -188,8 +211,8 @@ export interface HandlerWillRespondCallback {

export interface HandlerDidRespondCallbackParam {
request: Request;
response?: Response;
event?: ExtendableEvent;
response?: Response;
state?: PluginState;
}

Expand All @@ -199,9 +222,9 @@ export interface HandlerDidRespondCallback {

export interface HandlerDidCompleteCallbackParam {
request: Request;
response?: Response;
error?: Error;
event?: ExtendableEvent;
response?: Response;
state?: PluginState;
}

Expand All @@ -214,29 +237,29 @@ export interface HandlerDidCompleteCallback {
* cache operations.
*/
export interface WorkboxPlugin {
handlerWillStart?: HandlerWillStartCallback;
cacheDidUpdate?: CacheDidUpdateCallback;
cachedResponseWillBeUsed?: CachedResponseWillBeUsedCallback;
cacheKeyWillBeUsed?: CacheKeyWillBeUsedCallback;
cacheWillUpdate?: CacheWillUpdateCallback;
cachedResponseWillBeUsed?: CachedResponseWillBeUsedCallback;
fetchDidFail?: FetchDidFailCallback;
fetchDidSucceed?: FetchDidSucceedCallback;
requestWillFetch?: RequestWillFetchCallback;
handlerWillRespond?: HandlerWillRespondCallback;
handlerDidRespond?: HandlerDidRespondCallback;
handlerDidComplete?: HandlerDidCompleteCallback;
handlerDidRespond?: HandlerDidRespondCallback;
handlerWillRespond?: HandlerWillRespondCallback;
handlerWillStart?: HandlerWillStartCallback;
requestWillFetch?: RequestWillFetchCallback;
}

export interface WorkboxPluginCallbackParam {
handlerWillStart: HandlerWillStartCallbackParam;
cacheDidUpdate: CacheDidUpdateCallbackParam;
cachedResponseWillBeUsed: CachedResponseWillBeUsedCallbackParam;
cacheKeyWillBeUsed: CacheKeyWillBeUsedCallbackParam;
cacheWillUpdate: CacheWillUpdateCallbackParam;
cachedResponseWillBeUsed: CachedResponseWillBeUsedCallbackParam;
fetchDidFail: FetchDidFailCallbackParam;
fetchDidSucceed: FetchDidSucceedCallbackParam;
requestWillFetch: RequestWillFetchCallbackParam;
handlerWillRespond: HandlerWillRespondCallbackParam;
handlerDidRespond: HandlerDidRespondCallbackParam;
handlerDidComplete: HandlerDidCompleteCallbackParam;
handlerDidRespond: HandlerDidRespondCallbackParam;
handlerWillRespond: HandlerWillRespondCallbackParam;
handlerWillStart: HandlerWillStartCallbackParam;
requestWillFetch: RequestWillFetchCallbackParam;
}
10 changes: 6 additions & 4 deletions packages/workbox-routing/src/NavigationRoute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@

import {assert} from 'workbox-core/_private/assert.js';
import {logger} from 'workbox-core/_private/logger.js';
import {RouteHandler, RouteMatchCallbackOptions} from 'workbox-core/types.js';

import {Route} from './Route.js';
import {Handler, MatchCallbackOptions} from './_types.js';

import './_version.js';

export interface NavigationRouteMatchOptions {
Expand Down Expand Up @@ -55,7 +57,7 @@ class NavigationRoute extends Route {
* match the URL's pathname and search parameter, the route will handle the
* request (assuming the denylist doesn't match).
*/
constructor(handler: Handler,
constructor(handler: RouteHandler,
{allowlist = [/./], denylist = []}: NavigationRouteMatchOptions = {}) {
if (process.env.NODE_ENV !== 'production') {
assert!.isArrayOfClass(allowlist, RegExp, {
Expand All @@ -72,7 +74,7 @@ class NavigationRoute extends Route {
});
}

super((options: MatchCallbackOptions) => this._match(options), handler);
super((options: RouteMatchCallbackOptions) => this._match(options), handler);

this._allowlist = allowlist;
this._denylist = denylist;
Expand All @@ -88,7 +90,7 @@ class NavigationRoute extends Route {
*
* @private
*/
private _match({url, request}: MatchCallbackOptions): boolean {
private _match({url, request}: RouteMatchCallbackOptions): boolean {
if (request && request.mode !== 'navigate') {
return false;
}
Expand Down
9 changes: 6 additions & 3 deletions packages/workbox-routing/src/RegExpRoute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@

import {assert} from 'workbox-core/_private/assert.js';
import {logger} from 'workbox-core/_private/logger.js';
import {RouteHandler, RouteMatchCallback, RouteMatchCallbackOptions}
from 'workbox-core/types.js';

import {HTTPMethod} from './utils/constants.js';
import {Route} from './Route.js';
import {Handler, MatchCallbackOptions, MatchCallback} from './_types.js';

import './_version.js';


Expand Down Expand Up @@ -41,7 +44,7 @@ class RegExpRoute extends Route {
* @param {string} [method='GET'] The HTTP method to match the Route
* against.
*/
constructor(regExp: RegExp, handler: Handler, method?: HTTPMethod) {
constructor(regExp: RegExp, handler: RouteHandler, method?: HTTPMethod) {
if (process.env.NODE_ENV !== 'production') {
assert!.isInstance(regExp, RegExp, {
moduleName: 'workbox-routing',
Expand All @@ -51,7 +54,7 @@ class RegExpRoute extends Route {
});
}

const match: MatchCallback = ({url}: MatchCallbackOptions) => {
const match: RouteMatchCallback = ({url}: RouteMatchCallbackOptions) => {
const result = regExp.exec(url.href);

// Return immediately if there's no match.
Expand Down
11 changes: 6 additions & 5 deletions packages/workbox-routing/src/Route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
import {assert} from 'workbox-core/_private/assert.js';
import {HTTPMethod, defaultMethod, validMethods} from './utils/constants.js';
import {normalizeHandler} from './utils/normalizeHandler.js';
import {Handler, HandlerObject, MatchCallback} from './_types.js';
import {RouteHandler, RouteHandlerObject, RouteMatchCallback}
from 'workbox-core/types.js';
import './_version.js';


Expand All @@ -23,8 +24,8 @@ import './_version.js';
* @memberof module:workbox-routing
*/
class Route {
handler: HandlerObject;
match: MatchCallback;
handler: RouteHandlerObject;
match: RouteMatchCallback;
method: HTTPMethod;

/**
Expand All @@ -39,8 +40,8 @@ class Route {
* against.
*/
constructor(
match: MatchCallback,
handler: Handler,
match: RouteMatchCallback,
handler: RouteHandler,
method: HTTPMethod = defaultMethod) {
if (process.env.NODE_ENV !== 'production') {
assert!.isType(match, 'function', {
Expand Down
32 changes: 18 additions & 14 deletions packages/workbox-routing/src/Router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@

import {assert} from 'workbox-core/_private/assert.js';
import {getFriendlyURL} from 'workbox-core/_private/getFriendlyURL.js';
import {Handler, HandlerObject, HandlerCallbackOptions, MatchCallbackOptions}
from './_types.js';
import {
RouteHandler,
RouteHandlerObject,
RouteHandlerCallbackOptions,
RouteMatchCallbackOptions,
} from 'workbox-core/types.js';
import {HTTPMethod, defaultMethod} from './utils/constants.js';
import {logger} from 'workbox-core/_private/logger.js';
import {normalizeHandler} from './utils/normalizeHandler.js';
Expand Down Expand Up @@ -46,8 +50,8 @@ interface CacheURLsMessageData {
*/
class Router {
private readonly _routes: Map<HTTPMethod, Route[]>;
private readonly _defaultHandlerMap: Map<HTTPMethod, HandlerObject>;
private _catchHandler?: HandlerObject;
private readonly _defaultHandlerMap: Map<HTTPMethod, RouteHandlerObject>;
private _catchHandler?: RouteHandlerObject;

/**
* Initializes a new Router.
Expand Down Expand Up @@ -120,7 +124,7 @@ class Router {
}

const request = new Request(...entry);
return this.handleRequest({request});
return this.handleRequest({request, event});

// TODO(philipwalton): TypeScript errors without this typecast for
// some reason (probably a bug). The real type here should work but
Expand All @@ -142,17 +146,16 @@ class Router {
* appropriate Route's handler.
*
* @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.
* @param {Request} options.request The request to handle.
* @param {ExtendableEvent} options.event The event that triggered the
* request.
* @return {Promise<Response>|undefined} A promise is returned if a
* registered route can handle the request. If there is no matching
* route and there's no `defaultHandler`, `undefined` is returned.
*/
handleRequest({request, event}: {
request: Request;
event?: ExtendableEvent;
event: ExtendableEvent;
}): Promise<Response> | undefined {
if (process.env.NODE_ENV !== 'production') {
assert!.isInstance(request, Request, {
Expand Down Expand Up @@ -272,8 +275,9 @@ class Router {
* They are populated if a matching route was found or `undefined`
* otherwise.
*/
findMatchingRoute({url, sameOrigin, request, event}: MatchCallbackOptions):
{route?: Route; params?: HandlerCallbackOptions['params']} {
findMatchingRoute(
{url, sameOrigin, request, event}: RouteMatchCallbackOptions):
{route?: Route; params?: RouteHandlerCallbackOptions['params']} {
const routes = this._routes.get(request.method as HTTPMethod) || [];
for (const route of routes) {
let params;
Expand Down Expand Up @@ -317,7 +321,7 @@ class Router {
* @param {string} [method='GET'] The HTTP method to associate with this
* default handler. Each method has its own default.
*/
setDefaultHandler(handler: Handler, method: HTTPMethod = defaultMethod) {
setDefaultHandler(handler: RouteHandler, method: HTTPMethod = defaultMethod) {
this._defaultHandlerMap.set(method, normalizeHandler(handler));
}

Expand All @@ -328,7 +332,7 @@ class Router {
* @param {module:workbox-routing~handlerCallback} handler A callback
* function that returns a Promise resulting in a Response.
*/
setCatchHandler(handler: Handler) {
setCatchHandler(handler: RouteHandler) {
this._catchHandler = normalizeHandler(handler);
}

Expand Down
17 changes: 0 additions & 17 deletions packages/workbox-routing/src/_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,8 @@
https://opensource.org/licenses/MIT.
*/

import {
RouteHandler,
RouteHandlerObject,
RouteHandlerCallback,
RouteHandlerCallbackOptions,
RouteMatchCallback,
RouteMatchCallbackOptions
} from 'workbox-core/types.js';
import './_version.js';

export {
RouteHandler as Handler,
RouteHandlerObject as HandlerObject,
RouteHandlerCallback as HandlerCallback,
RouteHandlerCallbackOptions as HandlerCallbackOptions,
RouteMatchCallback as MatchCallback,
RouteMatchCallbackOptions as MatchCallbackOptions,
}

// * * * IMPORTANT! * * *
// ------------------------------------------------------------------------- //
// jdsoc type definitions cannot be declared above TypeScript definitions or
jeffposnick marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading