language-tools icon indicating copy to clipboard operation
language-tools copied to clipboard

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

Open dummdidumm opened this issue 4 years ago • 115 comments

[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:

  • It is not possible to define a generic relationship between props and events/slots (generics)
  • It is not possible to define the events and their types
  • It is not possible to define the slots and their types in a specific way

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

dummdidumm avatar Aug 11 '20 07:08 dummdidumm

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).

pngwn avatar Aug 11 '20 07:08 pngwn

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.

dummdidumm avatar Aug 11 '20 12:08 dummdidumm

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]
}

halfnelson avatar Aug 12 '20 07:08 halfnelson

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.

dummdidumm avatar Aug 12 '20 07:08 dummdidumm

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

I have to dash now, but one more thought: As for typing the slot, would JSX generics provide any inspiration?

  • https://mariusschulz.com/blog/passing-generics-to-jsx-elements-in-typescript
  • https://www.typescriptlang.org/docs/handbook/jsx.html

shirakaba avatar Aug 12 '20 09:08 shirakaba

Another quick one, as I read it in some of the related issues: I've had no end of trouble using JSDoc comments to type things, including the @template option.

While we should keep an open mind to JSDoc (even using JSDoc within the HTML), I must warn that, whether used through WebStorm or VS Code, it is simply not expressive as TypeScript. e.g. it does not implement the true type; I'm sure it doesn't do index types; and if I recall, you can't have a Record<keyof X, any> either. I keep running into walls with it. @template did work, but was pretty limited, too. And I think it works differently well depending on IDE, as well.

shirakaba avatar Aug 12 '20 09:08 shirakaba

Passing the generic type the (jsx)element is a no-go for me because it's not valid svelte template syntax. It should also not be needed since generics are driven by properties, I cannot think of a way to introduce a generic dependent for slots/events only. If they are driven by input properties passing generics to jsx elements is not needed because we can generate svelte2tsx code in such a way that TS will infer it for us.

About the typing overhead: This is correct but would only be needed in situations where you want to use generics and/or ecplicitly type events/slots. In theory, we could infer all of this ourselves, but this feels super hard to implement. Examples of such hard to implement problems are support for advanced slots scenarios like in sveltejs/svelte#263, and to collect all possible component events (which is easy if the user only would use createEventDispatcher, but he could also import a function which wraps createEventDispatcher stuff).

In general I feel that there are many situations where people would want only to define one of those things, but not the others, so maybe it is indeed necessary to provide all the different options to keep the "type less"-spirit of Svelte.

This would mean:

  • ComponentDef is the do-all-in-one-if-you-need-it
  • ComponentEvents is for typing events only
  • ComponentSlots is for typings slots only
  • a construct like <script generic="T"> if you only need generics
  • a combination of ComponentEvents/ComponentSlots/generic="T"

dummdidumm avatar Aug 12 '20 09:08 dummdidumm

@shirakaba The JSDoc type support doesn't have to be as feature-rich as a typescript. I doubt a lot of people would type generic components with it. Also because we're using typescript's language service, a lot of the advanced type can be easily imported from a typescript file.

About slot props type-check, I have some hack but don't know if this would lead to good developer experience. If user type component like this:

interface ComponentDef {
      slots: { default: { item: string } }
  }

we can generate a class

class DefaultSlot extends Svelte2TsxComponent<ComponentDef['slots']['default']>{ }

and transform default slot to

<DeafaultSlot />

jasonlyu123 avatar Aug 12 '20 14:08 jasonlyu123

(only chiming in as a user) For me the extra typing is not an issue because I have to do a lot of it anyway while working with TypeScript.

As for the interface vs the generic prop, since there are cases where variables are only exposed to the consumer, it would be better DX to support the prop. But on that note, would it possile to support export type T instead of generic="T"?

joelmukuthu avatar Aug 25 '20 09:08 joelmukuthu

This is invalid TS syntax which might confuse people. Also, we would have to do an additional transformation step, which would not be done in case the Svelte component is in a non-compilable-state. In this case the language-tools fall back to what is in the script, which then would have this invalid syntax. So I'm against that.

dummdidumm avatar Aug 25 '20 09:08 dummdidumm

I understand. Not to linger, but is it invalid syntax or more of an "undefined/unknown type" error? I'm asking because I don't know how TypeScript's compiler handles the two cases. I just have reservations against adding a custom attribute to a script tag, especially with a name as generic as "generic" :)

All in all, for me, this is a nice-to-have and perhaps forcing the user to type the exported variables when using the component is a good thing (more readable code).

joelmukuthu avatar Aug 25 '20 14:08 joelmukuthu

When you type export type T;, TS will throw a syntax error. Also, how to formulate constraints? export type T extends string; will throw more syntax errors at you. I totally understand your reservation against a custom attribute, but I think it's the least disrupting way. Other ways would be to have reserved type names like T1 or T2 but that is not flexible enough (how to add constraints?).

The alternative would be to type the whole component via ComponentDef, where you can freely add generics, like in the example in the starting post. But this comes at the cost of more typing.

For me it becomes clear from the discussion that people have very different opinions on this and to have more options laid out (ComponentDef, generic props as attributes, only typing ComponentEvents) is the most flexible to make most of them happy, although they introduce different ways of doing the same thing.

dummdidumm avatar Aug 25 '20 14:08 dummdidumm

I may have missed this but is splitting it up an option?

<script>
  import type { Foo } from './foo';
  export let items: Foo[];
  interface ComponentSlots<T> {}
  interface ComponentEvents<T> {}
</script>

Would that reduce the typing overhead for most cases?

joelmukuthu avatar Aug 26 '20 07:08 joelmukuthu

Yes this would be possible.

dummdidumm avatar Aug 26 '20 07:08 dummdidumm

@dummdidumm is it possible to work on supporting interface ComponentSlots {} (with or without support for generics) in the meantime? Or would introducing that be contradictory to the things discussed here?

joelmukuthu avatar Sep 18 '20 13:09 joelmukuthu

It wouldn't, but before continuing the implementation I first want to write a RFC to get more feedback from the community and the other maintainers on the proposed solution. Once there's an agreement, we will start implementing those things.

dummdidumm avatar Sep 18 '20 13:09 dummdidumm

@joelmukuthu btw why would you like to have the interface support? We enhanced the type inference for slots, is there a case where it's still not working out for you? If so, I'm curious to hear your case.

dummdidumm avatar Sep 20 '20 15:09 dummdidumm

I now created a RFC on this topic: https://github.com/sveltejs/rfcs/pull/38 Discussion on the API should continue there.

dummdidumm avatar Sep 20 '20 15:09 dummdidumm

@dummdidumm sorry for the slow response. I've just realised that for my use-case, I do actually need support for generics. Type inference for slots works quite well!

joelmukuthu avatar Sep 28 '20 16:09 joelmukuthu

This would be huge! I'm currently saddened that using slot properties is always an any.

NickClark avatar Nov 12 '20 22:11 NickClark

That's strange, slot typed can be infered quite nicely at this point, if the compont you use is within you project and uses TS, too.

dummdidumm avatar Nov 12 '20 22:11 dummdidumm

Perhaps I'm using the wrong terms here... or hopefully I just did something wrong.

Example of what I wish I could do:

<SelectInput options={myArrayOfObjects} let:option>
   <!-- I wish option here was the correct type */ -->
  {option.myLabelProp}
</SelectInput>

<!-- SelectInput Component -->

<script lang="ts" generic="T">
  export let options: Array<T> = [];
</script>

<div class="select">
  {#each options as option}
    <slot {option} />
  {/each}
</div>

NickClark avatar Nov 12 '20 22:11 NickClark

Ok I understand, yes right now this is not possible, so you have to fall back to any[]. If this would allow only string[], your slot would be typed as string, which is what I meant by "type can be infered".

dummdidumm avatar Nov 13 '20 07:11 dummdidumm

A nice temporary solution to type checking components I've come across in "Svelte and Sapper in Action" by Mark Volkmann is the use of the React prop-types library alongside $$props.

This is the code for a Todo item for example:

import PropTypes from "prop-types/prop-types";

const propTypes = {
    text: PropTypes.string.isRequired,
    checked: PropTypes.bool,
};

// Interface for Todo items
export interface TodoInterface {
    id: number;
    text: string;
    checked: boolean;
}

PropTypes.checkPropTypes(propTypes, $$props, "prop", "Todo");

While I'm validating the props of Todo and getting errors if something is off, I also want to have an interface for Todo items, so that I can have static type checking in VSCode as well as autocompletions. This is achieved by using the interface I defined above. The obvious and significant downside of this is that I need to have duplicate code here. I can't define an interface from the propTypes object, or vice-versa.

I'm still starting out in javascript so please correct my if any of this is wrong.

I think type checking is a must have for any productive codebase, both static checking and props type checking at runtime.

(Note that the interface would be defined in context='module' so its exportable alongside the component default export, but omitted that part for brevity)

Edit: Have not tried this for anything other than props validation, let me know if it would also work for slots/events!

RamiAwar avatar Feb 23 '21 08:02 RamiAwar

Experimental support for typing props/events/slots and generics is now available. See the RFC for details on how to use it. Please provide feedback in this issue.

dummdidumm avatar Jun 20 '21 16:06 dummdidumm

Worked first try for my first simple use case 👍

<script context="module" lang="typescript">
  let uniqueSelectId = 0
</script>

<script lang="typescript">
  type ITEM = $$Generic

  export let selectedItem: ITEM | undefined
  export let items: ITEM[]
  export let onChange: (item: ITEM) => void
  export let itemRenderer: (item: ITEM) => string

  export let label: string | undefined = undefined
  export let placeholder: string | undefined = undefined

  const uniqueId = `select-${uniqueSelectId++}`
</script>

<!-- svelte-ignore a11y-no-onchange -->
<select
  class="he-Select"
  name={uniqueId}
  bind:value={selectedItem}
  on:change={() => {
    if (selectedItem) onChange(selectedItem)
  }}>
  {#if placeholder}
    <option value="" disabled selected>{placeholder}</option>
  {/if}
  {#each items as item}
    <option value={item}>{itemRenderer(item)}</option>
  {/each}
</select>

{#if label}<label for={uniqueId}>{label}</label>{/if}

AlexGalays avatar Jun 20 '21 18:06 AlexGalays

It works brilliantly, even in a fairly advanced case!

<script lang="typescript">
    ...

    type A = $$Generic;
    type R = $$Generic;

    export let query: (
        client: ApolloClient<NormalizedCacheObject>,
        args: Omit<QueryOptions<A>, 'query'>
    ) => Readable<
        (ApolloQueryResult<R> | ApolloQueryResult<undefined>) & {
            query: ObservableQuery<R, A>;
        }
    >;
    export let variables: A | undefined = undefined;

    interface $$Slots {
        body: { data: R };
    }

    const queryRef = query(client, { variables });
</script>
...
<slot name="body" data={$queryRef.data} />
``

AndreasHald avatar Jun 25 '21 08:06 AndreasHald

@dummdidumm Hey there. Great job adding support for generics and stuff. But it seems like the support for the $$Events interface has created problems for when you use the generic param of createEventDispatcher() to declare event types. (It could also be the result of sveltejs/svelte#921, I don't really know)

Consider the following:

Timeline.svelte:

<script lang="ts">
    const dispatch = createEventDispatcher<{
        scrubbing: ScrubbingDetails;
        scrubbingEnd: ScrubbingDetails;
    }>();
</script>

The code completion doesn't work:

image

Also, even when you manually type in the event name, and you hover over it, you don't get a tooltip showing the type of the event.

Although the type-checking actually does work properly, so when you assign a badly typed handler to the event:

image

I'm assuming this has to do with the VS Code extension. (I'm on version v105.2.2, latest)

Note that you'll have none of these problems if you use the $$Events interface.

Should I create an issue for this or is this enough?

aradalvand avatar Jul 02 '21 22:07 aradalvand

I'm also wondering that now that the $$Events interface is here, does that mean we're supposed to stop using the type parameter of createEventDispatcher and instead rely on the new $$Events? Or is the $$Events interface only supposed to be used when there are events that are dispatched by something other than the returning function of createEventDispatcher?

aradalvand avatar Jul 02 '21 22:07 aradalvand

One more thing: If an event doesn't have details, should its type be any or void?

const dispatch = createEventDispatcher<{
    seekedTo: SeekedDetails;
    togglePlayback: any;
}>();

or:

const dispatch = createEventDispatcher<{
    seekedTo: SeekedDetails;
    togglePlayback: void;
}>();

I believe it should be any since CustomEvent (with type parameter not specified) is actually CustomEvent<any>.

Am I right?

aradalvand avatar Jul 02 '21 22:07 aradalvand