sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.41k stars 1.88k forks source link

When omitting $types, the type assigned to page load differs from PageLoad #12483

Open benblank opened 1 month ago

benblank commented 1 month ago

Describe the bug

When omitting $types from +page.ts, the return type of the load doesn't seem to be checked correctly. When typed as PageLoad, required properties in PageData which aren't present in the return type cause a type error. If the types are omitted (as described in the blog post), the type error no longer occurs.

Reproduction

  1. Run npm create svelte@latest repro && cd repro && npm install (select "Skeleton project", "Yes, using TypeScript syntax", and no additions).
  2. Run npm run check — no errors or warnings, yet.
  3. Edit src/app.d.ts; uncomment PageData and give it the properties { foo: string; bar: string; }.
  4. Create src/routes/+page.ts with the contents:

    import type { PageLoad } from './$types';
    
    export const load: PageLoad = () => {
     return { foo: "exists" };
    };
  5. Run npm run check again — this time, there's a type error saying that "Property 'bar' is missing …" (shown in full in the "Logs" section).
  6. Edit src/routes/+page.ts, removing the import and type annotation.
  7. Run npm run check one more time — the type error is no longer reported.

(The same behavior can be seen when using VS Code instead of running svelte-check directly.)

Logs

> omit-types-repro@0.0.1 check /home/five35/Documents/tmp/omit-types-repro
> svelte-kit sync && svelte-check --tsconfig ./tsconfig.json

====================================
Loading svelte-check in workspace: /home/five35/Documents/tmp/omit-types-repro
Getting Svelte diagnostics...

/home/five35/Documents/tmp/omit-types-repro/src/routes/+page.ts:3:14
Error: Type '() => { foo: string; }' is not assignable to type 'PageLoad'.
  Type '{ foo: string; }' is not assignable to type 'MaybePromise<Omit<PageData, never> & Partial<Pick<PageData, never>> & Record<string, any>>'.
    Type '{ foo: string; }' is not assignable to type 'Omit<PageData, never> & Partial<Pick<PageData, never>> & Record<string, any>'.
      Property 'bar' is missing in type '{ foo: string; }' but required in type 'Omit<PageData, never>'. 

export const load: PageLoad = () => {
  return { foo: "exists" };

====================================
svelte-check found 1 error and 0 warnings in 1 file
 ELIFECYCLE  Command failed with exit code 1.

System Info

System:
    OS: Linux 6.8 Ubuntu 24.04 LTS 24.04 LTS (Noble Numbat)
    CPU: (4) x64 Intel(R) Core(TM) i7-5600U CPU @ 2.60GHz
    Memory: 2.64 GB / 7.63 GB
    Container: Yes
    Shell: 5.2.21 - /bin/bash
  Binaries:
    Node: 20.13.1 - ~/.local/share/asdf/installs/nodejs/20.13.1/bin/node
    Yarn: 1.22.22 - ~/.local/share/asdf/installs/nodejs/20.13.1/bin/yarn
    npm: 10.5.2 - ~/.local/share/asdf/plugins/nodejs/shims/npm
    pnpm: 9.5.0 - ~/.local/share/asdf/installs/nodejs/20.13.1/bin/pnpm
  npmPackages:
    @sveltejs/adapter-auto: ^3.0.0 => 3.2.2 
    @sveltejs/kit: ^2.0.0 => 2.5.18 
    @sveltejs/vite-plugin-svelte: ^3.0.0 => 3.1.1 
    svelte: ^4.2.7 => 4.2.18 
    vite: ^5.0.3 => 5.3.4

Severity

serious, but I can work around it

Additional Information

I'm new to Svelte and SvelteKit, so it's possible this is user error or expected behavior, but I wasn't able to find anything in the documentation or the blog article which indicates that the type of load should be anything other than PageLoad. (In fact, export const load: PageLoad = … is used in an example on the types page.)

eltigerchino commented 1 month ago

This is expected behaviour. You're correct that the universal load function should be typed with PageLoad, but you should only add properties to the PageData type in src/app.d.ts if they are used for every load function. Otherwise, you can make the property optional so it doesn't error when a load function doesn't return the property.

benblank commented 1 month ago

Yeah, I realized I was misusing PageData after creating this issue. 😅

I don't think I understand what you're saying about the universal load function, though. If the only change is removing the explicit PageLoad typing, and that changes whether a type error appears, doesn't that mean that it isn't getting that type when being automatically typed by the tooling?

eltigerchino commented 1 month ago

Yes, it isn’t getting typed by the definition in app.d.ts if you remove it, but it doesn’t seem like having the property required for all load functions was what you wanted. Do you mind sharing what your original intention was?

benblank commented 1 month ago

My original intent was misguided, but that wasn't why I created the issue. At this point, I just want to be able to use zero-config typing and have confidence that it's actually type-checking things correctly. Both the documentation and the article imply that zero-config typing is equivalent to explicit typing, but if switching from one to the other changes what type errors occur, it doesn't seem like they are equivalent.

As a more concrete example, imagine a site which uses a styleMode property to track whether a visitor wants to use dark more or light mode. Any component on any page may need to adjust its styling based on that mode, so it gets added to PageData and because the decision regarding that styling always needs to be made, it's a required property.

Now, if styleMode isn't present in the return type of a load() function which has been explicitly given the type PageLoad (e.g. export const load: PageLoad = () => ({})), then a type error occurs. But if a load() function doesn't have an explicit type (e.g. just export const load = () => ({})), the zero-config type assigned to it seems to be something other than PageLoad, because no type error occurs even though a required property is missing.

That's all the repro in the issue description is trying to demonstrate. Set up an explicitly-typed load() function which causes a type error, then switch from explicit typing to zero-config. Poof, no more error, even though the function is still returning the wrong type.

eltigerchino commented 1 month ago

Sorry, that's my mistake. I completely misunderstood the issue. You're right, it should be throwing out a type error even when it's not explicitly typed (although I'm not sure if this should be fixed in Kit or the Svelte VS Code extension? cc: @dummdidumm ).

Also, not sure if we can do anything about this, but a clearer error message about which property is missing would be helpful when the load function is explicitly typed and the PageData type is declared. The TS error message when not returning anything doesn't really help in this regard.

Type '() => void' is not assignable to type 'PageLoad'.
  Type 'void' is not assignable to type 'MaybePromise<Omit<PageData, never> & Partial<Pick<PageData, never>> & Record<string, any>>'.ts(2322)