Skip to content

Add Typescript type definitions for workbox-sw#1436

Closed
wessberg wants to merge 8 commits into
GoogleChrome:masterfrom
wessberg:master
Closed

Add Typescript type definitions for workbox-sw#1436
wessberg wants to merge 8 commits into
GoogleChrome:masterfrom
wessberg:master

Conversation

@wessberg

@wessberg wessberg commented Apr 16, 2018

Copy link
Copy Markdown

R: @jeffposnick @addyosmani @gauntface

Fixes #1410

This PR adds Typescript type definitions for workbox-sw and provides them from the types property of the package.json of that package.

The type declarations are complete and based on the reference docs.

Also, during linting, ESLint complained about a few invalid JSDoc comments in some other files, so I've also fixed those.

@googlebot

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@wessberg

Copy link
Copy Markdown
Author

CLA has been signed

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

@coveralls

coveralls commented Apr 16, 2018

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.2%) to 86.833% when pulling 09d2f7e on wessberg:master into 116a873 on GoogleChrome:master.

@prateekbh

prateekbh commented Apr 16, 2018

Copy link
Copy Markdown
Collaborator

Also, just an fyi and to check if there is no collision between the two.

The TS for workbox generateSW options is here: Polymer/polymer-build#309

@wessberg

Copy link
Copy Markdown
Author

@prateekbh There are no collisions between the two 😊. Your typings cover workbox-build whereas these strictly cover workbox-sw and related namespaces.

@arkhamvm

Copy link
Copy Markdown

It's will be great! Any news about adding?

@wessberg

Copy link
Copy Markdown
Author

I added typings for streams such that the typings fully support v3.2.0

@josephliccini

Copy link
Copy Markdown
Contributor

Awesome to see TypeScript support!!

Though, I think there might be a bug in these:
image

Shouldn't it be Promise<Response> | undefined, not Promise<Response | undefined>?

@josephliccini

Copy link
Copy Markdown
Contributor

Also, it would be cool if we could get an interface for custom plugins to implement that would have the type checking enabled. It would basically enforce that any plugins are of this shape: https://developers.google.com/web/tools/workbox/guides/using-plugins#custom_plugins

@josephliccini

Copy link
Copy Markdown
Contributor

Here are the plugin typings:

// Custom type for Officehome
declare interface WorkboxPlugin {
    readonly cacheWillUpdate?: (context: CacheWillUpdatePluginContext) => Response
    readonly cacheDidUpdate?: (context: CacheDidUpdatePluginContext) => void;
    readonly cachedResponseWillBeUsed?: (context: CacheResponseWillBeUsedPluginContext) => Response
    readonly requestWillFetch?: (context: RequestWillFetchPluginContext) => Request;
    readonly fetchDidFail?: (context: FetchDidFailPluginContext) => void;
};

interface CacheWillUpdatePluginContext {
    readonly request: Request;
    readonly response: Response;
}

interface CacheDidUpdatePluginContext {
    readonly cacheName: string;
    readonly request: Request;
    readonly oldResponse: Response;
    readonly newResponse: Response;
}

interface CacheResponseWillBeUsedPluginContext {
    readonly cacheName: string;
    readonly request: Request;
    readonly matchOptions: any; // <-- What should this be? `object`?
    readonly cachedResponse: Response;
}

interface RequestWillFetchPluginContext {
    readonly request: Request;
}

interface FetchDidFailPluginContext {
    readonly originalRequest: Request;
    readonly request: Request;
    readonly error: Error;
}

Also, had to modify this type:

type Plugin = BroadcastUpdatePlugin | RangeRequestsPlugin | CacheableResponsePlugin | BackgroundSyncPlugin | ExpirationPlugin | WorkboxPlugin;

@wessberg

wessberg commented May 24, 2018

Copy link
Copy Markdown
Author

@josephliccini Yes, you're right. I fixed the signature of handleRequest. Thanks!

I also added typings for Custom Workbox Plugins (Exported as a named export). Thanks for your typings! I corrected them a little bit (cacheWillUpdate can return Response OR null, and cachedResponseWillBeUsed can return Response OR null)

@josephliccini

Copy link
Copy Markdown
Contributor

Hey @wessberg looks like custom plugin callbacks that return values support returning Promises as well.

#1507

you can see it used in the source here:
https://github.com/GoogleChrome/workbox/blob/685e81673d5c190209137dcaf6871f9fd5d98ce5/packages/workbox-range-requests/Plugin.mjs

and for requestWillFetch:

request = await plugin[pluginEvents.REQUEST_WILL_FETCH].call(plugin, {
request: request.clone(),
});

So looks like we should update typings for custom plugins to support Response, null, or Promise<Response | null>

@jeffposnick

Copy link
Copy Markdown
Contributor

Hello @wessberg!

First off, apologies for the delay in acting on this PR.

After discussing it with the other current maintainers, we don't feel like we can commit to maintaining these typings over time. We appreciate the work you put into creating them to begin with, but stale typings seem inevitable as we make changes to the codebase given our lack of familiarity with TypeScript.

I'm familiar enough with TypeScript to know that some folks submit "unofficial" .d.ts entries to https://definitelytyped.org/, and if you're willing to, I'd encourage you to go that route. We'd be happy to add a badge to our docs linking to your submission for other developers who want to find them.

Would you be willing to go that route?

@wessberg

wessberg commented Jun 14, 2018

Copy link
Copy Markdown
Author

@josephliccini, I've updated the typings to support returning Promise<Response> from cachedResponseWillBeUsed and cacheWillUpdate. Nice catch!

@jeffposnick, that is completely understandable 👍. I'll try to add the typings to DefinitelyTyped instead such that they can be installed from @types/workbox-sw.

@jeffposnick

Copy link
Copy Markdown
Contributor

I'm going to close this PR in favor of the DefinitelyTyped approach.

Once there's something published that you want us to link to, pass along the URL.

@wessberg

Copy link
Copy Markdown
Author

@jeffposnick, the typings have been published now!
Here's the URL: https://www.npmjs.com/package/@types/workbox-sw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Link to TypeScript definitions for workbox-sw

7 participants