Make it possible to type context and props for LoadInput and LoadOutput

[dummdidumm]

• added generics for Context and Props
• added a helper type "MaybePromise" to not write T | Promise all the time

Fixes #1442

@puchesjr is this what you had in mind? Usage would look like this:

<script context="module">
  import { LoadInput, LoadOutput } from '@sveltejs/kit';
  import { YourInputContextTypeDefinition, YourOutputContextTypeDefinition, YourPropsTypeDefinition } from './somewhere'

  export function load(input: LoadInput<YourInputContextTypeDefinition>): Promise<LoadOutput<YourPropsTypeDefinition, YourOutputContextTypeDefinition> {
    // ...
  }
</script>

We could bikeshed if the output context generic should appear before the props generic.

[ignatiusmb]

Since only Load is exposed publicly, perhaps it would look something like

<script context="module">
  import { Load } from '@sveltejs/kit';
  import { InputContextType, OutputContextType, PropsType } from './somewhere'

  export const load: Load<InputContextType, OutputContextType, PropsType> = async ({ context }) => {
    // ...
    return {
      props: {},
      context: {}
    }
  }
</script>

[utkarshkukreti]

Might be slightly off-topic: Are there any thoughts on using unknown more in place of any, e.g. Record<string, unknown>? That way the user has to explicitly specify or cast the type when in strict mode, adding a little more type safety.

[ignatiusmb]

Using unknown might cause inconvenience for existing users that don't pass any type arguments. I would expect those using strict mode would just pass in their types to get the type safety.

[benmccann]

Is this sort of necessary as a pre-requisite to #647? As an end-user, I think what's described there is less verbose. We should think about how these efforts would interact. If the user is typing out the types we might come to a different conclusion on named versus positional arguments than if they're autogenerated

[dummdidumm]

It's a prerequisite, yes, and it means we need even more generics for input, namely one for params, which makes me lean even more towards named arguments. I think for the autogenerated types it doesn't really matter if they are named or positional, they can deal with both.

[puchesjr]

• added generics for Context and Props
• added a helper type "MaybePromise" to not write T | Promise all the time

Fixes #1442

@puchesjr is this what you had in mind? Usage would look like this:

<script context="module">
  import { LoadInput, LoadOutput } from '@sveltejs/kit';
  import { YourInputContextTypeDefinition, YourOutputContextTypeDefinition, YourPropsTypeDefinition } from './somewhere'

  export function load(input: LoadInput<YourInputContextTypeDefinition>): Promise<LoadOutput<YourPropsTypeDefinition, YourOutputContextTypeDefinition> {
    // ...
  }
</script>

We could bikeshed if the output context generic should appear before the props generic.

Yes, that is in essence what I was eluding to. Not sure why @benmccann closed my issue so didn't initially see this reply/tag...

[dummdidumm]

I changed this to using two generic parameters - one for the input, one for the output - which each have named arguments. I think this feels more natural than having one big generic for both inputs and outputs. It also avoids the "how to name input/output context" question. I also added the params generic which is need for #647 . I had to add a type helper to infer the correct default value in case a parameter of a named generic is not set. It may make sense to add some type tests later on.

Once #984 is decided we have to rename the named arguments introduced in here.

[ignatiusmb]

👍 Looks good! I like the type helpers.

[ignatiusmb]

I think it's also a good idea to add type tests, especially early on to catch type changes more easily. I suggest, maybe we use tsd for it?

[benmccann]

I think the test code is type checked since #1319 so if we just add types to a couple of the existing tests that should cover it

[ignatiusmb]

On second thought, can we wait a bit for this. Quoting from a certain moderator in the TypeScript server

if you're building an API around manually specified generic params, that's usually a red flag and you might be misusing generics.

I was also wondering why IntelliSense didn't pick up correctly inside the Load<{ /* here */ }, { /* and here */ }>

[ignatiusmb]

As for the type tests using svelte-check, maybe @dummdidumm can chime in on this since I'm uncertain if it can do stuff like ExpectType or ExpectAssignable tests and such.

[dummdidumm]

On svelte-check - think of it as tsc for Svelte files, so no. That's why I also think dedicated type tests would be good.
On generics and named arguments: I tend to agree with that statement, but the problem remains - there's so much to type at once, so if we don't want 4 arguments, I don't see another way.

[ignatiusmb]

After multiple shower thoughts, I think it's best if we export LoadInput and LoadOutput here, each with their own 2 type arguments, and keep Load as is. This way, anyone can choose whether to use just Load with named type arguments, or use LoadInput and LoadOutput individually with the expected positional type argument. Best of both worlds?

[benmccann]

@dummdidumm it looks like this one will need to be rebased

On svelte-check - think of it as tsc for Svelte files, so no.

I'm not sure I understand why that wouldn't help. If we have Svelte files with load functions in them then we'd know if they adhere to the types or not. Is there something else you want to test?

[dummdidumm]

The current tests are testing the type correctness in a very rough way. I'd like to be more granular about it, and adding e2e tests for that purpose is wrong. Also I can't test that I expect a specific kind of error. That's why there's value in specific type tests.

[ignatiusmb]

Can we also add void in the return type of Load to fix #1606, or at least merge this so others can use the MaybePromise helper types.

[janosh]

Excuse me if this is a dumb question: Aren't the current LoadInput/LoadOutput incompatible with __error.svelte? Specifically LoadInput seems to be missing error and status and LoadOutput needs the ability to return nothing to fall through to the error page.

[dummdidumm]

There's ErrorLoadInput for the input part. For the output part - isn't the empty object also working? Either way you may be right if undefined is a valid return for this, since that's not typed on Load

[janosh]

There's ErrorLoadInput for the input part.

My bad, I missed that.

A bare return statement, I think, better communicates the intent of falling through to the error page than return {} so adding undefined to the return type would be great.

[dummdidumm]

Just checked, this was already done in #1617 , you are allowed to do return; when using the Load type.

[janosh]

@dummdidumm Could you show a usage example?