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

App-level types #3569

Closed
Rich-Harris opened this issue Jan 27, 2022 · 12 comments
Closed

App-level types #3569

Rich-Harris opened this issue Jan 27, 2022 · 12 comments

Comments

@Rich-Harris
Copy link
Member

Rich-Harris commented Jan 27, 2022

Describe the problem

Various types in a Kit app are app-level, by which I mean they're the same wherever they're used — event.locals, event.platform, session, and arguably stuff (which, even though it's scoped to load, can be used globally as $page.stuff and is therefore probably best thought of and used as a consistently-shaped object, where input stuff and output stuff in load are both Partial<Stuff>).

It's wasteful, therefore, that to take full advantage of type safety it's necessary to use generic arguments everywhere:

<script context="module">
  /**
   * @type {import('@sveltejs/kit').Load<{
   *   pageParams: PageParams;
   *   session: { foo: number };
   *   stuff: Stuff;
   * }, {
   *   props: Props;
   *   stuff: Stuff
   * }>} */
  export function load({...}) {...}
</script>
/**
 * @type {import('@sveltejs/kit').RequestHandler<
 *   Locals,
 *   Platform,
 *   Output
 * >}
 */
export function get({...}) {...}
/**
 * @type {import('@sveltejs/kit').Handle<
 *   Locals,
 *   Platform
 * >}
 */
export async function handle(...) {...}

Aside: why is it pageParams rather than params?

It's also a problem that RequestHandler and Handle take positional generic arguments — it meant that the addition of Platform was a breaking change, for example. And why doesn't RequestHandler accept params? If we fixed that, it would be another breaking change.

With app-level types (and assuming we were to treat stuff as app-level), the examples above could be rewritten thusly:

<script context="module">
  /**
   * @type {import('@sveltejs/kit').Load<{
   *   pageParams: PageParams;
   *   props: Props;
   * }>} */
  export function load({...}) {...}
</script>
/** @type {import('@sveltejs/kit').RequestHandler<Output>} */
export function get({...}) {...}
/** @type {import('@sveltejs/kit').Handle} */
export async function handle(...) {...}

Stretch goal — no manual typing at all

A priori, it's silly that you have to type params — SvelteKit already has that information. It should make it available... somehow.

In an ideal world, it wouldn't be necessary to type props either — that would be inferred from the props in the <script> block.

In an even idealer world, TypeScript would support implicit module typing and you wouldn't even have to declare types.

I've no idea how feasible any of this is in the real world (maybe there's something sneaky/clever we could do with preprocess?) but this is the development experience we should be aiming for, even if we fall short.

Describe the proposed solution

I'm not much of a TypeScript expert, so I don't really know how we would do this. @dummdidumm had a suggestion:

// src/app.d.ts
declare namespace SvelteKit {
  export interface Session { .. }
}
// inside SvelteKit
declare module "$app/stores" {
  export let session: Writable<SvelteKit.Session>
}

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

@benmccann benmccann added this to the 1.0 milestone Jan 27, 2022
@dummdidumm
Copy link
Member

dummdidumm commented Jan 29, 2022

I found a working solution which might be even more idiomatic than the namespace approach:

  1. Create the following helper type
type WithAnyFallback<T> = T extends never ? any : T;

Edit: Turns out we don't even need this, TS falls back to any for types it does not know.
2. Add the following to for example $app/stores:

declare module '$app/stores' {
  // ..
  // @ts-ignore
  export let session: Writable<Session>; // pre-edit: Writable<WithAnyFallback<Session>>
}

Session is not defined anywhere so this is a type error (which is why we add @ts-ignore above), but thanks to the helper type it will fall back to any
3. Add the following to the user's global.d.ts:

declare module '$app/stores' {
  export interface Session {
     whatever: 'i want';
   }
}

TypeScript will merge the module declarations, which means Session is now defined, which means intellisense will now use that type instead of any.

So the way forward would be to

  • define that helper type
  • enhance global.d.ts in create-svelte with initially empty module declarations for every declaration we support to type that way with some additional comments on how to take advantage of this

@Rich-Harris
Copy link
Member Author

I'd argue that the namespace is more idiomatic. It so happens that Session is used by $app/stores, but the session argument in load functions also needs to be typed, as do locals and stuff and platform which aren't available via a store.

Separately, I think app.d.ts might be a better name than global.d.ts, since it's the language we use elsewhere, though I don't feel strongly on that point.

Rich-Harris added a commit that referenced this issue Feb 2, 2022
* add a SvelteKit namespace for app-level types (#3569)

* update templates

* update docs

* fix some test types

* get typechecking to pass

* document app-level types

* changeset

* link to typescript section

* rename namespace to App

* update changeset

* shut up eslint no-one likes you

* point to docs

* remove Locals from todos/_api default template

* update adapter-cloudflare docs

Co-authored-by: Ignatius Bagus <ignatius.mbs@gmail.com>
@Rich-Harris
Copy link
Member Author

App-level stuff is implemented in #3670, and I've updated #647 to reflect the current state of things, so I'll close this

@biltongza
Copy link

biltongza commented Feb 5, 2022

I'm having trouble with this change. The moment there is an import in my app.d.ts file, the App namespace stops getting picked up.

You can see in this screenshot (of the default skeleton app made by svelte-init) that with the import commented out, there are 3 references to Stuff, 4 references to Session, and 1 reference to Locals and Platform:
image

Yet when I uncomment it, those references go away, and it's like Typescript pretends the namespace doesn't exist:
image

You get the same behaviour simply by exporting an interface in the same file:

image

However, not exporting it brings it back to life:
image

@dummdidumm
Copy link
Member

This is how typescript works. The d.ts file stops being an ambient module file as soon as you add a top level export or import, so they stop adding these types to the global types. So you should avoid doing that. Maybe we should add a comment in the starter template making this more clear. This also was why I preferred "global" as a file name.

@biltongza
Copy link

So then how do I reference a type that isn't in the same file?

@dummdidumm
Copy link
Member

For reusable types you want to import, create a separate d.ts file.

@biltongza
Copy link

I have exactly that, but the app.d.ts doesn't know about the types in the other file, unless I am missing something.

image

image

@jasonlyu123
Copy link
Member

jasonlyu123 commented Feb 6, 2022

you can import inside the namespace block. Or use import('./thing').Manifest as type

@biltongza
Copy link

Importing inside the namespace works! Thank you.

@jasonlyu123
Copy link
Member

Ah. Import inside a namespace isn't right. That only works inside the declare module.

@aradalvand
Copy link
Contributor

aradalvand commented Feb 8, 2022

I'm having issues with the type importing thing as well.
This is my app.d.ts:

/// <reference types="@sveltejs/kit" />

declare namespace App {
    interface Session extends import('ts-essentials').DeepWritable<import('./someFile').BasicSessionInfo> {
        isAuthenticated: boolean;
    }
}

But it's not working, do generics fail to work when using dynamic imports like this?! Or am I doing something wrong?

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

No branches or pull requests

6 participants