sveltejs / language-tools

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

Typing Svelte Component Props/Events/Slots [Feedback Thread] #442

Closed dummdidumm closed 3 months ago

dummdidumm commented 4 years ago

[this is now the feedback thread]

This was implemented according to this RFC. Please provide feedback about using it in this thread.

Original Post

There are several issues about this already (#424, sveltejs/svelte#304, sveltejs/svelte#273, sveltejs/svelte#263), but I wanted to make a consolidated one to have a discussion about the approaches we can take. Related PR: sveltejs/svelte#437

Note that this issue is NOT about typing a d.ts file, this will come afterwards as a separate issue.

Is your feature request related to a problem? Please describe. At the moment it is not possible to type the inputs/outputs of a component under certain circumstances:

Describe the solution you'd like A way to type props/events/slots explicitly.

Proposal: A new reserved interface ComponentDef which, when defined, is used as the public API of the component instead of infering the things from the code.

Example (with comments about what likely is not possible):

<script lang="ts"
   interface ComponentDef<T> { // <-- note that we can use generics as long as they are "defined" through props
      props: {  items: T[]  }
      events: {  itemClick: CustomEvent<T>  }
      slots: { default: { item: T } }
  }

   // generic type T is not usable here. Is that a good solution? Should it be possible?
   // Also: How do we make sure the types match the component definition?
  export items: any[];

   // ...

   // We cannot make sure that the dispatched event and its type are correct. Is that a problem?
   // Maybe enhance the type definitions in the core repo so createEventDispatcher accepts a record of eventname->type?
   dispatch('itemClick', item);
</script>
<!-- ... -->
<slot item={item}> <!-- again, we cannot make sure this actually matches the type definition -->

When someone now uses the component, he will get the correct types from the props/events/slots and also error diagnostics when something is wrong:

<Child
     items="{['a', 'string']}"
     on:itemClick="{event => event.detail === 1 /* ERROR, number not comparable to type string */}"
/>

If this works and is tested a little, we could enhance it with other reserved interfaces like ComponentEvents to only type one of the things.

I'd like to get as much feedback on this as possible because this is likely a big change which might be used broadly. Also, I'd like to have some of the core team members to have a look at this if this looks good to them or introduces something they don't agree with.

@jasonlyu123 @orta @Conduitry @antony @pngwn

pngwn commented 4 years ago

Seems reasonable, there is actually a Svelte issue about being able to inject slots at runtime. https://github.com/sveltejs/svelte/issues/2588 and a PR that implements it https://github.com/sveltejs/svelte/pull/4296, feels like there might be overlap, or at least some opportunity to align the interfaces (if there is any consenus, there are still some outstanding questions with the above PR).

dummdidumm commented 4 years ago

Thanks for the PR link, seems like it's only related in a way to us that we have to type the constructor a bit differently then, but I think this is indendent of the type checking on a template level.

halfnelson commented 4 years ago

interesting. I wonder if we could do something like

<script lang="ts" generic="T"> 
    type T = unknown
    export let items: T[]
    let item:T = items[0]
</script>
<slot b={item}></slot>

which would strip the type T = unknown during typecheck and instead add it as a generic argument to the component.

//...
render<T>() {
    export let items: T[]
    let item:T = items[0]
}
dummdidumm commented 4 years ago

Good idea about adding it to the render function!

I think we can get the same results with

<script lang="ts">
    interface ComponentDef<T> {
       ...
    } 
    type T = unknown
    export let items: T[]
    let item:T = items[0]
</script>
<slot b={item}></slot>

by extracting the T from the interface definition.

Although with your solution you will have to type less in the case of "I just want a generic relationship between my props and slots", which is nice. On the one hand I'm thinking "yeah we could just add both", on the other hand it feels a little like expanding the API surface too much (you can do the same things in different ways) - not sure.

shirakaba commented 4 years ago

I really don't want to have to write out the interface ComponentDef<T>{ props: {} } and line it up with each one of my exports. Svelte does so well in reducing characters typed compared to React, and this feels like a step backwards. In particular, it requires duplicating the types of every export into the props, which is no fun (and bound to lead to frequent problems).

I like @halfnelson's line of thinking. Exports should be detected as props. I don't know what the module looks like once Githubissues.

  • Githubissues is a development platform for aggregating issues.