sveltejs / language-tools

The Svelte Language Server, and official extensions which use it
MIT License
1.25k stars 200 forks source link

Svelte-check and VSCode extension errors regarding component types... How to declare them in Svelte 5? #2522

Closed charlie-roosen closed 4 days ago

charlie-roosen commented 1 month ago

Here is the way that I've been declaring components that I'm binding to a JavaScript reference via "bind:this":

<script lang="ts">
    import CloneWorkflowDialog from './dialogs/CloneWorkflowDialog.svelte';

   let cloneWorkflowDialog : CloneWorkflowDialog;   
</script>

<CloneWorkflowDialog bind:this={cloneWorkflowDialog}/>

With svelte-check 4.0.3, I'm getting new errors that are not flagged by 4.0.2:

c:\cr-bitbucket\RMP Platform Scala\w2e-svelte\svelteui\src\lib\components\workflow\WorkflowTitlePanel.svelte:102:30
Error: 'CloneWorkflowDialog' refers to a value, but is being used as a type here. Did you mean 'typeof CloneWorkflowDialog'? (ts)

   let cloneWorkflowDialog : CloneWorkflowDialog;

This is also being displayed as an error by "Svelte for VS Code" v109.0.2 and not by v108.6.1.

Is this a bug in the language tools, or has the idiom for declaring the types of components change?

jrmajor commented 1 month ago

It's probably related to #2517. I encountered a similar error and tried to add typeof, as suggested in the PR description, but got another error: Type 'SvelteComponent<$$ComponentProps, { [evt: string]: CustomEvent<any>; }, {}> & { $$bindings?: "" | undefined; } & { setPhotoUri: (uri: string, source: string) => void; }' is not assignable to type '__sveltets_2_IsomorphicComponent<$$ComponentProps, { [evt: string]: CustomEvent<any>; }, {}, { setPhotoUri: (uri: string, source: string) => void; }, "">'. In my case both components are using runes.

wheeOs commented 1 month ago

yep, I have the same issue. The VS Code extension complains, the check fails, but the app still runs and compiles

wheeOs commented 1 month ago

I did a rollback to v109.0.1 for now, and keep it so until this gets fixed

dummdidumm commented 1 month ago

This works correctly. In Svelte 5, components are functions. The types reflect that, so the default export is a function now. To type "I expect an instance of this function", you now do this:

<script lang="ts">
    import CloneWorkflowDialog from './dialogs/CloneWorkflowDialog.svelte';

-   let cloneWorkflowDialog : CloneWorkflowDialog;
+   let cloneWorkflowDialog : ReturnType<typeof CloneWorkflowDialog>;
</script>

<CloneWorkflowDialog bind:this={cloneWorkflowDialog}/>
bdmackie commented 1 month ago

@dummdidumm they've been functions for most of the RC. Is it necessary for it to be that verbose? Sorry if this sounds ranty but this already felt tedious/noisy, especially when triggered by not being able to reference 'this' component, and this makes it even more boilerplatey IMO. At least glad I saw this as I was pulling my hair out wondering why VS code caught on fire with red squiggles after a version bump.

Edit: I've read the related issue so have a vague idea that your hand was forced.... still if I can vote for some kind of ergonomics improvement for this requirement please... maybe even some compiler help?

P.S. I've been getting the 'non reactive state' warning and so to shush it I have been using syntax like this:

let editor1 = $state<Editor | null>(null);

So what you are saying is I need to do something like this:

let editor1 = $state<ReturnType<typeof Editor> | undefined>();

P.P.S. I even messed with Component<> types to try to match the component thinking that would be a cleaner way to do this, but I couldn't get a type definition that was assignable for the binding. I believe that is what the Component<> type is for? I suggest it's an anti-pattern to require the instance of an object to get its type.

Edit: struck out the point about using state as it's incorrect. The last point is what matters.

ndom91 commented 1 month ago

@dummdidumm We just came across this issue in our project as well. The updated syntax works and the svelte LSP no longer complains, however svelte-check still doesn't seem to be catching these issues. Meaning that these issues are getting through CI, but then being flagged by the LSP in the editor. So svelte-check probably needs updated as well, right?

PatrickG commented 1 month ago

Is it possible for the language-tools to export the typeof X type as default? So that as a user you don't have to write it.

// virtual/transformed code for `SomeComponent.svelte`
function Component() { ... }
type Component = typeof Component;
export default Component;
<script lang="ts">
  import SomeComponent from './SomeComponent.svelte';

  let instance: ReturnType<SomeComponent>;
  // or with https://github.com/sveltejs/svelte/pull/13441
  let instance: ComponentExports<SomeComponent>;
...
dummdidumm commented 1 month ago

I briefly thought about that, but the problem with that is that it would no work when you would declare the type by hand. And that could be very confusing ("why can I use the import as a type from a Svelte component, but if I declare a component using the Component type myself, I can't?")

charlie-roosen commented 1 month ago

Hi everyone and thanks for the feedback.

I've changed my code to use "ReturnType".

I'll go ahead and close the question.

ptrxyz commented 2 weeks ago

How does this work with generics? Consider this:

(1) A generic component like this:

<script lang="ts" generics="T extends { foo: string }">
    let { opt }: { opt: T } = $props()

    export function action(): T {
        // do stuff here
        return opt
    }
</script>

(2) Used like so:

<script lang="ts">
    import Comp from './comp.svelte'

    let instance: ReturnType<typeof Comp>

    const opt = {
        foo: 'bar',
        other: 1
    }

    function _callAction() {
        return instance.action().other    // this should not error if instance was properly typed.
    }
</script>

<Comp bind:this={instance} {opt}></Comp>

My problem is that I have no idea how to type instance to properly reflect that opt indeed has the other property. Somehow I have to add the generic parameter to instance to make it work, but I can't wrap my head around this to express it in Typescript.

Can anyone help?

dummdidumm commented 2 weeks ago

We're gonna implement the automatic type Foo = ReturnType<typeof Foo> trick within language tools so people don't run into this problem as often.