preload typings

[dummdidumm]

closes #1463

I added the typings to the d.ts file and put it in @sapper/common which seemed fitting to me since it is used on both client and server. I also added an alternative way of typing the function like @jasonlyu123 suggested, but did not mention it in the docs (I can add that if you want to).
I also added a doc entry.

[benmccann]

this looks pretty good to me. I wouldn't mind getting someone else to take a look too since we're introducing new API that's exposed to the user

[Jayphen]

Would it be appropriate to use the node-fetch types here instead of typing opts and res as any?

    fetch: (url: RequestInfo, init?: RequestInit) => Promise<Response>;

[benmccann]

That seems like it would probably work for Response:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/07b2759140ad390e14801cfcc5ed9ff94182e7bb/types/node-fetch/index.d.ts#L167

For the options, node-fetch extends the whatwg/fetch standard options with its own:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/07b2759140ad390e14801cfcc5ed9ff94182e7bb/types/node-fetch/index.d.ts#L46

That would only work on the server and I'm not sure we'd want to expose the underlying implementation to that degree. Perhaps we should just define our own interface for that?

export interface FetchOpts {
    body?: BodyInit;
    headers?: HeadersInit;
    method?: string;
    redirect?: RequestRedirect;
    signal?: AbortSignal | null;
}

Or maybe we can find types for the browser fetch that we can use instead?

[Jayphen]

Ah, I didn't realise node-fetch was a superset of the fetch standard. In that case it's safer to use the equivalent signature for whatwg-fetch

https://microsoft.github.io/PowerBI-JavaScript/modules/_node_modules_typedoc_node_modules_typescript_lib_lib_dom_d_.html#fetch

[Jayphen]

… in which case I think fetch: WindowOrWorkerGlobalScope["fetch"] would work

[Jayphen]

Just wondering — why wasn’t this typed in the end?

[dummdidumm]

As mentioned below I felt there was no concensus yet on what the typings should be. Feel free to open another PR for a dedicated discussion around this and possibly changing it.

[benmccann]

would WindowOrWorkerGlobalScope work on the server-side? I'm not that familiar with typescript, but it sounds like a browser thing

[Jayphen]

I think this looks good. Apart from the typing of fetch as I commented above, I've been using basically the same types in my own apps. I'm unaware of a better way to handle the types here.

[ehrencrona]

It would be good if we could also use these types in the actual sapper code to guarantee that we conform to them. I did a stab at that in this branch and it did reveal that we actually don't :) (it's possible to pass in a Request rather than just a string as the first parameter of fetch; also the headers can be of different types, though I didn't solve that cleanly).

I did wind up duplicating PreloadContext into types.ts because I couldn't figure out how to import it from index.d.ts.

@dummdidumm Feel free to merge my branch into this one if you agree with the changes.

[ehrencrona]

Sorry, one last comment. I find myself unable to use these typings in project code. I set up a TypeScript Sapper project using these instructions, but when importing either @sapper/app or @sapper/common I get Cannot find module '@sapper/common' or its corresponding type declarations.ts(2307).

I haven't been able to figure out how this is supposed to work. My guess would have been that we need a type attribute in the package.json, but changing that doesn't fix it for me.

[tommywalkie]

@ehrencrona I had the same issue, I simply had to link the types field to sapper/runtime...

"types": ["svelte", "node", "sapper/runtime"],

because this is where the index.d.ts file is.

sapper/runtime/index.d.ts

Lines 1 to 3 in e97846c

	declare module "@sapper/app"
	declare module "@sapper/server"
	declare module "@sapper/service-worker"

[dummdidumm]

If I remember correctly there's some copy-script that copies these typings into src/node_modules which should make them available. But that's not part of this PR.

[dummdidumm]

How to move forward? I updated the PR with all suggestions except the fetch typing which seemed to me that there is no concensus yet.

[benmccann]

@dummdidumm thanks for pushing this through. I just had one more question about naming, but this looks good to me

[benmccann]

thanks for all your work on this!! this is a great improvement

[btakita]

Thank you for releasing the types! I'm getting an error when referencing the Page type. I added "sapper/common" to the tsconfig.json types.

import { Page } from '@sapper/common'
export type $page_type = Page;

TS2709: Cannot use namespace 'Page' as a type.
TS4081: Exported type alias '$page_type' has or is using private name 'Page'.

[ignatiusmb]

Sorry to comment on a merged PR, but both variables doesn't seem to be used below, shouldn't it be Preload?

[dummdidumm]

Good catch, yes this is wrong, it should be Preload.

[dummdidumm]

Fixed on master