Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

preload typings #1468

Merged
merged 10 commits into from
Oct 2, 2020
Merged

preload typings #1468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b467361
preload typings
Sep 1, 2020
54bac86
enhanced docs
Sep 1, 2020
bb5e157
import type
Sep 1, 2020
438042b
update typings
dummdidumm Sep 23, 2020
d69cc32
PreloadPage -> Page
dummdidumm Sep 29, 2020
acc0a0c
PreloadFunction -> Preload
dummdidumm Oct 1, 2020
217ee66
Update site/content/docs/04-preloading.md
benmccann Oct 1, 2020
3a3f268
Update site/content/docs/04-preloading.md
benmccann Oct 1, 2020
bdbb0d4
Update site/content/docs/04-preloading.md
benmccann Oct 1, 2020
40941b7
Update site/content/docs/04-preloading.md
dummdidumm Oct 2, 2020
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
21 changes: 21 additions & 0 deletions runtime/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
declare module "@sapper/app"
declare module "@sapper/server"
declare module "@sapper/service-worker"
declare module "@sapper/common"

declare module "@sapper/app" {
export interface Redirect {
Expand Down Expand Up @@ -35,3 +36,23 @@ declare module "@sapper/service-worker" {
export const shell: string[];
export const routes: Array<{ pattern: RegExp }>;
}

declare module "@sapper/common" {
export interface PreloadContext {
fetch: (url: string, options?: any) => Promise<any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

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>;

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@Jayphen Jayphen Sep 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

error: (statusCode: number, message: Error | string) => void;
redirect: (statusCode: number, location: string) => void;
}

export interface Page {
host: string;
path: string;
params: Record<string, string>;
query: Record<string, string | string[]>;
error?: Error;
}

export interface Preload {
(this: PreloadContext, page: Page, session: any): object | Promise<object>;
}
}
20 changes: 20 additions & 0 deletions site/content/docs/04-preloading.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,3 +132,23 @@ You can abort rendering and redirect to a different location with `this.redirect
}
</script>
```

#### Typing the function

If you use TypeScript and want to access the above context methods, TypeScript will thow an error and tell you that it does not know the type of `this`. To fix it, you need to specify the type of `this` (see the [official TypeScript documentation](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html#specifying-the-type-of-this-for-functions)). We provide you with helper interfaces so you can type the function like this:

```html
<script context="module">
import type { Page, PreloadContext } from "@sapper/common";
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed on master


export const preload: Preload = async function(this, page, session) {
const { user } = session;

if (!user) {
return this.redirect(302, 'login'); // TypeScript will know the type of `this` now
}

return { user };
}
</script>
```