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

Typing props and stuff in LoadOutput and Load #3392

Closed
aradalvand opened this issue Jan 18, 2022 · 10 comments · Fixed by #3455
Closed

Typing props and stuff in LoadOutput and Load #3392

aradalvand opened this issue Jan 18, 2022 · 10 comments · Fixed by #3455

Comments

@aradalvand
Copy link
Contributor

aradalvand commented Jan 18, 2022

Describe the bug

I'm using TypeScript, and trying to specify the type of the props property in the return type of my load function. But I don't seem to be able to get it working:

<script lang="ts" context="module">
    import type { Load, LoadInput, LoadOutput } from "@sveltejs/kit";

    type Props = {
        foo: number;
    }

    const output: LoadOutput<Props> = {
        props: { // type is 'Props', as expected.

        }
    }

    export const load: Load<LoadInput, LoadOutput<Props>> = async () => {
        return {
            props: { // type is 'Rec<any>', it should be 'Props'.

            }
        }
    };
</script>

Also, when I do:

type Test = ReturnType<Load<LoadInput, LoadOutput<Props>>>>;
// 'Test' is:
// MaybePromise<Either<LoadOutput<Rec<any>, Rec<any>>, Fallthrough>>
// But it should be:
// MaybePromise<Either<LoadOutput<Props, Rec<any>>, Fallthrough>>

There seems to me to be something wrong with SvelteKit's Load type here.

System Info

  System:
    OS: Windows 10 10.0.19042
    CPU: (4) x64 Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz
    Memory: 1.02 GB / 7.89 GB
  Binaries:
    Node: 14.15.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.10 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 6.14.8 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Spartan (44.19041.1266.0), Chromium (97.0.1072.62)
    Internet Explorer: 11.0.19041.1202
  npmPackages:
    @sveltejs/kit: ^1.0.0-next.232 => 1.0.0-next.232
    svelte: ^3.42.6 => 3.44.2

Severity

annoyance

@aradalvand aradalvand changed the title Typing props in LoadOutput Typing props and stuff in LoadOutput and Load Jan 18, 2022
@aradalvand
Copy link
Contributor Author

aradalvand commented Jan 18, 2022

Update: After playing with this for hours, I just found out that the way you're supposed to type the properties of input and output of a load function (like props for the output type, or even session of the input type) is like this:

export const load: Load<{ session: SessionType }, { props: PropsType }> = async () => {
    return {
        props: {

        }
    }
};

This is the first time I'm seeing this sort of thing but I understand the rationale behind it, it is brilliant, as it allows you to selectively choose the type of each of the properties, something that's not possible with regular TypeScript generic parameters. But at first glance it wasn't clear to me that you're supposed to do it like this at all; and I'm pretty sure I'm not alone in this. I asked a question about this in the Discord server and the answer I got from one of the moderators made the very same mistake that I made — here.

So I'm going to keep this issue open as a suggestion to write about this in the docs.

@7nik
Copy link

7nik commented Jan 18, 2022

The interface LoadOutput<> defines props as optional, but the inner type InferValue<> expects props to be a required property, so it fallbacks to Rec<any> instead of Props.

You can see why it happens here:

import type { InferValue, Rec } from "@sveltejs/kit/types/helper";
// the InferValue returns either type of the named property in the first type or the third type
type Test1 = InferValue<LoadOutput<Props>, 'props', Rec>; // type Test1 = { [x: string]: any; }
// the same but with just unfolded and simplified, for this case, `LoadOutput`.
type Test2 = InferValue<{props?: Props}, 'props', Rec>; // type Test2 = { [x: string]: any; }
type Test3 = InferValue<{props: Props}, 'props', Rec>; // type Test3 = { foo: number; }

But LoadInput doesn't have this problem as all its fields are defined as required.

@aradalvand
Copy link
Contributor Author

aradalvand commented Jan 18, 2022

@7nik Thanks for the explanation. I was confused about that specific point too, although I didn't mention it, now it makes sense.
I still think it would be nice if this was mentioned somewhere perhaps, as it's likely to confuse people.

@7nik
Copy link

7nik commented Jan 20, 2022

I doubt that the PR fixed the problem. As far as I can see, the types passed to LoadOutput<> still get ignored and replaced with Record<string, any>.
I think the solution can be to return the following type by Load:

MaybePromise<
    Either<
	    Fallthrough,
	    LoadOutput<
		    InferValue<Required<Output>, 'props', Record<string, any>>,
		    InferValue<Required<Output>, 'stuff', Record<string, any>>
	    >
    >
>

It's should be okay to requirefy Output as LoadOutput anyway defines props and stuff as optional.

@aradalvand
Copy link
Contributor Author

@ignatiusmb Sorry I don't understand how your PR resolved this issue?!

@ignatiusmb
Copy link
Member

What seems to be the problem here? The original issue points that you don't understand how to use the Load type from the lack of documentation, hence the PR including the definitions in the docs.

@aradalvand
Copy link
Contributor Author

aradalvand commented Jan 21, 2022

@ignatiusmb Sure but I was expecting an example (for the usage, if you will), as opposed to just the type definitions.
Similar to the code snippet in this comment of mine.
I don't think people can immediately understand that this is how Load should be used just from looking at the type definitions.

@ignatiusmb
Copy link
Member

We don't have any usage examples for using types in the docs, so it doesn't make sense to add one for this.

@aradalvand
Copy link
Contributor Author

aradalvand commented Jan 21, 2022

Okay then I think it would be a good idea to add type usage examples to the docs in general, although I understand that that's beyond the scope of this issue. I don't really think type definitions in the docs are of much help, they're hard to read and parse and look like noise, honestly.
Is there an already-existing issue for this?

@ignatiusmb
Copy link
Member

ignatiusmb commented Jan 21, 2022

Is there an already-existing issue for this?

I suppose not, but that shouldn't hopefully be an issue once we get to #3324

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

Successfully merging a pull request may close this issue.

4 participants