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

TypeScript: Explicitly Typing Props/Slots/Events + Generics #38

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm changed the title TS: Typing Props/Slots/Events TypeScript: Explicitly Typing Props/Slots/Events + Generics Sep 20, 2020
@numfin
Copy link

numfin commented Sep 27, 2020

Why not use 1 interface for props/events/slots ? This way we can create events, that depends on props:

interface ComponentDefinition<T extends Record<string, string>> {
  props: { a: T },
  events: { b: T }
}

p.s. Sorry for bad english, tried my best.

@dummdidumm
Copy link
Member Author

Yes this would be possible through the ComponentDef interface

@stefan-wullems
Copy link

For the slots, props and events, I would lose the Component prefix:

  • ComponentSlots -> Slots
  • ComponentProps -> Props
  • ComponentEvents -> Events

Then finally ComponentDef -> Component.

Doesn't make the names a lot more ambiguous or prone to conflict with existing types I think.

@stefan-wullems
Copy link

stefan-wullems commented Sep 28, 2020

Perhaps separating the type definition from the logic of a component would work nicely.

<script lang="ts" definition>
  interface Component<T> {
     props: { value: T }
     events: { 
       change: (value: T) => void
     }
     slots: {
       default: {}
     }
  }
</script>

<script lang="ts">
  export let value
</script>

<some>
  <slot>Markup</slot>
</some>

@dummdidumm
Copy link
Member Author

I like the shortening of the names although I think this might increase the possibility of name collisions.
I'm against putting this into a separate script. This would require more effort in preprocessing, also I like the colocation of the interfaces within the instance script.

@AlexGalays
Copy link

Can't wait to use this <3

  • Basic components without generics means mapping domain models from/to arbitrary component specific structures.
  • Passing a component to a slot is impossible so our best workaround for now would be to pass a [Component, ComponentProps] tuple but there's no way to have ComponentProps mean anything right now.

🙏

@dummdidumm
Copy link
Member Author

Could you elaborate on your second point a little more? I'm don't fully understand the use case. And what do you mean by "impossible"? Possible to do in Svelte but the type checker complains?

@AlexGalays
Copy link

(unless I missed something)

you can't pass a component instance to a slot so people end up either

  • wrapping all their slots with a div on the call site, ending up in a div soup or
  • passing a component and its props as a non slot prop: <ParentComponent renderHeader={[SomeSvelteComponent, Props]} />

but today, we have no way to express that Props should be SomeSvelteComponent's Props beside triple checking manually.

@francoislg
Copy link

Stumbled upon this and just wanted to throw here a slight variation of Option 2:

<script lang="ts">
    import {createEventDispatcher} from "svelte";

    type T = ComponentGeneric<boolean>; // extends boolean
    type X = ComponentGeneric; // any

    export let array1: T[];
    export let item1: T;
    export let array2: X[];
    const dispatch = createEventDispatcher<{arrayItemClick: X}>();
</script>

I think it would be slightly closer to TypeScript code than a ComponentGenerics interface that gets magically expanded 😄

@denis-mludek
Copy link

denis-mludek commented Dec 9, 2020

Hi !

Any idea when Generic Component will be available/released? Is it perhaps a case of choice paralysis? I think we would all love something even if it's not 100% perfect!!

I'm working on a SvelteTS project for several weeks now, and I would have used this feature a few times already.
Btw, love the work you're doing to make SvelteTS a thing. TS support was the thing that made me switch from React to Svelte. 🤗

@dummdidumm
Copy link
Member Author

@tomblachut tagging you since you are the maintainer of the IntelliJ Svelte Plugin - anything in that proposal that concerns you implementation-wise ("not possible to implement on our end")?

@tomblachut
Copy link

@dummdidumm thank you for tagging me.

Generics will definitely be, as you've written, an uncanny valley and maintenance burden.

Option 1 & 2 without Svelte support in editor will produce invalid TS, given that both will require special reference resolution. I think it's better to avoid that.

I'd scratch Option 2, because ComponentGenerics is in the same scope as things that will refer to its type parameters. I imagine it will add some implementation complexity AND mental overhead for users.

I quite like Option 3 because it's valid TS. ComponentGeneric would be treated as identity mapped type.

    type T = ComponentGeneric<boolean>; // extends boolean
    type X = ComponentGeneric; // any

Option 3 could be even simplified a bit by giving new semantics to export type in similar way as export let denotes a prop

    export type T = boolean;
    export type X = any;

Now, I think it's better to stick to one style of declarations: (separate interfaces/compound ComponentDef/namespace) otherwise we may introduce small bugs in one of them and more importantly will need to decide on and teach about precedence.

One additional thing this proposal does not mention is ability to extend interfaces. I think that's great feature. Author of the component could say "this "PromotedPost adheres to Post props" and whenever types are changed in Post definition, implementing components would show type errors. Unless I'm missing something interfaces will support that use case out of the box.

@dummdidumm
Copy link
Member Author

dummdidumm commented Feb 20, 2021

Thanks for your insights!

I agree that we should only provide one style of declarations. Separate interfaces feels like the best option there.

I also agree that for generics option 3 feels the closest to vanilla TypeScript which is why I prefer that, too. That simplification does not feel quite right for me though, because we are not exporting that generic to anyone, we are just stating that the component is generic.
Option 3 has one little shortcoming though, being not being strict enough. Take this code snippet:

type T = ComponentGeneric<{a: boolean}>;
const t: T = {a: true};

Without extra Svelte-specific typing-work, this snippet would not error, because TS does not think of T as a generic. If it did, it would error with Type '{ a: true; }' is not assignable to type 'T'. '{ a: true; }' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint '{ a: boolean; }'. In general, errors related to T would not be in the context of generics. I'd say this is a hit we can take though, because the types are "good enough" for most use cases, and with some extra transformation work (like svelte2tsx) you can even make TS think that this is a generic - and also it's just not possible to make TS think of T as a generic at the top level without transformations.

One thing you brought up about extending interfaces is a very good advantage, but it also got me thinking how to deal with generics in that context.

For example you have this interface:

export interface ListProps<ListElement> {
   list: ListElement[];
}

How do you extend it while keeping it generic in the context of a Svelte component? The only possibility that comes to my mind is to do this:

<script lang="ts">
  import type { ListProps } from '..';

  type BooleanListElement = ComponentGeneric<boolean>;
  interface ComponentProps extends ListProps<BooleanListElement> {}

  export let list: BooleanListElement[];
</script>
..

@non25
Copy link

non25 commented Feb 24, 2021

I miss generics too.
I'll leave one example which I would really appreciate to use generics with:

This checkbox selector returns subset of an array of objects without mutating them, which is really handy, but it will return any[] instead of passing types forward, which is sadge... 😞

<script>
  import Checkbox from '../components/Checkbox.svelte';

  export let checkboxes = [];
  export let checked = [];
  export let idF = 'id';
  export let textF = 'text';

  let checkedIds = new Set(checked.map((c) => c[idF]));

  function mark(id) {
    checkedIds = new Set(
      checkedIds.has(id)
        ? [...checkedIds].filter((cid) => cid !== id)
        : [...checkedIds, id]
    );
  }

  $: checked = checkboxes.filter((c) => checkedIds.has(c[idF]));
</script>

<ul class="checkboxes">
  {#each checkboxes as checkbox (checkbox[idF])}
    <li>
      <Checkbox
        checked={checkedIds.has(checkbox[idF])}
        on:change={() => mark(checkbox[idF])}
        desc={checkbox[textF]}
      />
    </li>
  {/each}
</ul>

This is proposed way to use generics? I don't think I understood examples correctly, so I added type definitions to this component.

<script lang="ts">
  import Checkbox from '../components/Checkbox.svelte';

  type Item = ComponentGeneric; // how this will attach to the checkboxes prop ?

  export let checkboxes: Item[] = [];
  export let checked: Item[] = [];
  export let idF = 'id';
  export let textF = 'text';

  let checkedIds = new Set<number>(checked.map((c: Item) => c[idF]));

  function mark(id: number) {
    checkedIds = new Set(
      checkedIds.has(id)
        ? [...checkedIds].filter((cid: number) => cid !== id)
        : [...checkedIds, id]
    ):
  }

  $: checked = checkboxes.filter((c: Item) => checkedIds.has(c[idF]));
</script>

@dummdidumm
Copy link
Member Author

Everything inside this proposal is type-only, which means it's only there to assist you at compile time to find errors early - nothing of this proposal will be usable at runtime, similar to how all TypeScript types will be gone at runtime.

In your example you would do:

<script lang="ts">
  // ..
  type Item = ComponentGeneric;
  type ItemKey = ComponentGeneric<keyof Item>;

  export let checkboxes: Item[] = [];
  export let checked: Item[] = [];
  // Note: For the following props, the default value is left out because you cannot expect "id" or "text" to be present as a default if you don't narrow the Item type
  export let idF: ItemKey;
  export let textF: ItemKey;
// ..

@non25
Copy link

non25 commented Feb 24, 2021

Wow, keyof is cool.

I want to make sure that I understand correctly, so here's another example:

<script lang="ts">
  interface Vegetables {
    id: number;
    name: string;
    weight: number;
  }

  let someItems: Vegetables[] = [
    ...
  ];

  let checked: Vegetables[] = [];
</script>

<CheckboxSelector checkboxes={someItems} textF="name" idF="id" bind:checked />
<!--                            ^                ^ 
                                |                | Could it attach to this?
                                |
                                | how do I specify that ComponentGeneric
                                  should attach to this?
-->

For example here's how I would use this in regular typescript, which is clear to me.

function component<T>(checkboxes: T[] = [], idF: <keyof T>, textF: <keyof T>) {
  ...
}

@dummdidumm
Copy link
Member Author

When using the component, you don't specify anything, you just use the component and the types should be inferred and errors should be thrown if the relationship between is incorrect. So in your example if you do textF="nope" it would error that nope is not a key of Vegetables.

Doing

type T = ComponentGeneric;
export let checked: T[];
export let idF: keyof T;
// ...

Would be in the context of Svelte components semantically the same as

function component<T>(checked: T[], idF: keyof T) {
  ...
}

This is the uncanny valley I'm talking about in the RFC which I fear is unavoidable - it doesn't feel the same like generics, yet it serves this purpose inside Svelte components (and inside only Svelte components). The problem is that Svelte's nice syntax of just directly starting with the implementation without needing something like a wrapping function, so there is no place to nicely attach generics.

@non25
Copy link

non25 commented Feb 24, 2021

So if I end up in a situation where I need two generics:

function component<X, Y>(items: X[], someOtherProp: Y) {
  ...
}

How that would look in the proposed approach? 🤔
Is it even possible ?

Do you know how bindings will work in the current state?

In the example above I added annotation that checked in parent component is of same type as someItems, but if we pass someItems through checkboxes prop, we should lose the type, because we can only use any as 'generic' type now in the CheckboxSelector component, right ?

Do annotations to a bind override any[] in this case ? 🤔

Sorry for the wording...

@dummdidumm
Copy link
Member Author

So if I end up in a situation where I need two generics:

function component<X, Y>(items: X[], someOtherProp: Y) {
  ...
}

How that would look in the proposed approach? 🤔
Is it even possible ?

You do this

type X = ComponentGeneric;
type Y = ComponentGeneric;
export let items: X[];
export let someOtherProp: Y;

Do you know how bindings will work in the current state?

In the example above I added annotation that checked in parent component is of same type as someItems, but if we pass someItems through checkboxes prop, we should lose the type, because we can only use any as 'generic' type now in the CheckboxSelector component, right ?

Do annotations to a bind override any[] in this case ? 🤔

Sorry for the wording...

Sorry, I don't know what you mean.

@non25
Copy link

non25 commented Feb 24, 2021

Sorry, I don't know what you mean.

I hope this is a better way to explain. 🤔

<script lang="ts">
  interface Vegetables {
    id: number;
    name: string;
    weight: number;
  }

  let someItems: Vegetables[] = [
    ...
  ];

  // I annotate same type to the checked prop, which I will bind below
  // What type checked will have after the bind ?
  let checked: Vegetables[] = [];
  // I need to use checked in other places and want it to retain the type
</script>

<CheckboxSelector checkboxes={someItems} textF="name" idF="id" bind:checked />
<!--                                                                  ^
                                                          binding checked here
-->

<script lang="ts">
  // ...
  // I set any here because I want to accept any object type
  // and generics is currently not supported
  export let checkboxes: any[] = [];

  // this prop will contain a subset of checkboxes prop,
  // which I bind above
  export let checked: any[] = [];
  // ...
</script>

@wvhulle
Copy link

wvhulle commented May 27, 2023

If the generics version works well and can do all the same or more, it is fine for me. But I cannot just throw away my existing generic components (which have been used in production for almost a year) until this is resolved, so I reverted to eslint svelte3 which still works as expected.

Apart from that, @dummdidumm sample code seems to have valid arguments for the generics attribute.

I understand the $$Generic angular bracket syntax is not very consistent with other Typescript language constructs. Isn't there an alternative to using an html attribute for this? Perhaps an @ sign or something else that makes it clear this is not just a normal html property or JavaScript?

Maybe keep both options, $$Generic and generics open?

@Nick-Mazuk
Copy link

Nick-Mazuk commented May 30, 2023

How would records in the generic attribute work?

<script lang='ts' generics='T extends { foo: string }'>

</script>

Normally curly braces in HTML mean to substitute the contents with with JS expression. But here we don't want the substitution and foo: string is not a valid JS expression.

FWIW, I tried this with the change in sveltejs/language-tools#2020 released in v3.4.3 and am getting the following error:

Error: Expected } (svelte)
<script lang="ts" generics="T extends { key: string }">

I could hoist the { key: string } into the module script, but that seems like a workaround that shouldn't exist.

<script lang='ts' context='module'>
    type Foo = { foo: string }
</script>

<script lang='ts' generics='T extends Foo'>

</script>

I already filed sveltejs/language-tools#2039 to keep track of this but posting here as well in case we want to discuss the desired solution.

@TheOnlyTails
Copy link

I imagine that you would have to escape the curly braces as normal with &123; and &125;.

@brunnerh
Copy link
Member

It probably does not make sense to try and interpolate here, the value should be constant at compile time.

I imagine that you would have to escape the curly braces as normal with &123; and &125;.

I would much rather just do this, if really necessary:

<script lang='ts' generics={'T extends { foo: string }'}>

@TGlide
Copy link
Member

TGlide commented May 30, 2023

Also, how would the generics attribute work with events & slots? Would it also be an attribute?

If so, it really worries me legibility wise compared to what we have now:
Screenshot 2023-05-30 at 17 52 29

@dummdidumm
Copy link
Member Author

The generics defined on the generics attribute are available to the whole instance script content, including $$Events and $$Slots.

The record thing is a good catch, the Svelte parser thinks it's a mustache tag when it shouldn't. Mhm we may need to special-case this inside the parser, or add a change to the typescript preprocessor where it strips the generics attribute. The latter is now easily possible in Svelte 4, so that's probably the most promising route.

@wvhulle
Copy link

wvhulle commented May 30, 2023

@dummdidumm will it be possible to use the imported types inside the script instance tag inside the generics attribute?

And will it be possible to use type aliases defined in the script instance tag inside the generics tag?

@TGlide
Copy link
Member

TGlide commented May 31, 2023

The generics defined on the generics attribute are available to the whole instance script content, including $$Events and $$Slots.

The record thing is a good catch, the Svelte parser thinks it's a mustache tag when it shouldn't. Mhm we may need to special-case this inside the parser, or add a change to the typescript preprocessor where it strips the generics attribute. The latter is now easily possible in Svelte 4, so that's probably the most promising route.

I see, thanks for the answer! Though I think I expressed myself badly, I meant to ask if $$Events and $$Slots will also be replaced by an attribute?

@AdrianGonz97
Copy link
Member

Hey folks,

I just wanted to note that I love the new generics attribute for one particular reason that I have yet to see anyone comment on (I've read from March to today).

I'm not aware of any way to have Generic Parameter Defaults with $$Generic (if there's a way, I'd greatly appreciate being enlighten 😄).

Since Svelte components are compiled down into classes, we should have a way to specify default generic types like so:

class SomeSvelteComponent<T extends SomeType = SomeDefaultType> { ... }

But I don't think $$Generic can permit that.

With the generics attribute, we can simply specify the default like so:

<script lang="ts" generics="T extends SomeType = SomeDefaultType">

and it works beautifully!
(the only exception being that if you put a typeof someFunction for the default type, the syntax highlighter gets all wonky)

@ptrxyz
Copy link

ptrxyz commented Jun 23, 2023

I also have trouble getting things to work with generics=".....". Some examples include:

// case 1: parsing error, expected `}`
<script lang="ts" generics="T extends { id: string }, Y extends string">

// case 2: unexpected end of input"
<script lang="ts" generics="T extends Record<string, number>, Y extends string">

In both cases the type does not resolve and ends up as any

@ptrxyz
Copy link

ptrxyz commented Jun 28, 2023

I found this: sveltejs/language-tools#2039

@Nick-Mazuk
Copy link

Anyone else having issues with svelte-check not type checking props when using the generic attribute?

I filed sveltejs/language-tools#2107 just in case, but figured someone here may have figured out a way to get it working.

@tzuleger
Copy link

Is there hope for JSDOC typing to see support for this in the future?

@mythril
Copy link

mythril commented Aug 22, 2023

I'm just an end-user but, generics as a string attribute seems very out of place to me.

I don't see why it would be preferable to do that vs a new tag that has some restrictions on it, and essentially hoists the component into a class definition.

for example a context="type" tag which can only contain type info, a component attribute to give the component a name in the context to work with, a truncated (no body?) class definition, and a token to represent the generated component code

<script lang="ts" context="type" component="SomeSvelteComponent">
  class SomeSvelteComponent<T extends SomeType = SomeDefaultType> { ...$$Component }
</script>
<script lang="ts">
  // regular component definition here
  export let value: string = '';
</script>

I think this is sort of like the first option "ComponentDefinition" approach, but it seems to me that it could almost be a text-replacement macro, applied after the component is generated, then type checked. Though I don't really understand the depths of the maintenance burden discussion, nor have I ever even looked at the compiler and how it works, so sorry if this is just noise.

@mythril
Copy link

mythril commented Aug 22, 2023

Now that I think of it this might provide a path for extendable components.

@mythril
Copy link

mythril commented Aug 22, 2023

And alternate type systems like flow, jsdoc, and typescript

kelvindecosta added a commit to kelvindecosta/eslint-config that referenced this pull request Apr 14, 2024
antfu pushed a commit to antfu/eslint-config that referenced this pull request Apr 15, 2024
jade-gjz added a commit to jaderd-jh/eslint-config that referenced this pull request Apr 16, 2024
Bernankez added a commit to Bernankez/eslint-config that referenced this pull request Apr 18, 2024
jade-gjz added a commit to jaderd-jh/eslint-config that referenced this pull request May 11, 2024
jade-gjz added a commit to jaderd-jh/eslint-config that referenced this pull request May 11, 2024
* refactor: migrate to antfu's config

* chore: remove format feature

* chore: rename all configs' name with `jhqn` prefix

* fix: cli-suggest-remove-files

* fix: ignore `.yarn` folder

* chore: remove `eslint-plugin-react-refresh`

* feat: add config compat

* feat: add config regexp

* feat: improve cli

* chore: replace `eslint-plugin-x` with `eslint-plugin-import-x`

* fix(cli): git clean check

* feat: support more fields of packageJson as ascending order

* feat: automatically rename plugins in factory

* fix: support eslint v9

* feat: graphql glob that supports .qgl extensions

* feat: support flat config pipeline

* chore: update deps

* feat: improve types for rules

* chore: update typegen

* feat: more relax types for merging

* fix(cli): make frameworks not required

* fix: consistent on config names

* docs: move to `@eslint/config-inspector`

* feat: generate types for core rules as well

* chore: update deps

* feat: update names for all config items

* fix: move `no-new-symbol` to `no-new-native-nonconstructor`

* feat: try inspector build

* chore: try fix windows

* feat: support solid.js

* docs: mention Solid support

* feat: improve `no-unused-vars` options

* chore: update dev script

* fix: don't create new test plugin for every run

* chore: update deps

* chore: remove old configs

* ci: codesandbox

* ci: codesandbox

* ci: codesandbox

* fix(stylistic): turn off 'antfu/top-level-function'

* docs: rename composer

* fix: what if enable stylistic by default and remove all config

* Revert "fix: what if enable stylistic by default and remove all config"

This reverts commit 6313724.

* fix: adjust `unused-imports/no-unused-vars`

* chore: update deps

* feat: improve types support

* feat(svelte): add support for typing according to sveltejs/rfcs#38

* feat: support `lessOpinionated` option

* refactor: migrate from eslint-plugin-react to eslint react

* chore: update deps

* fix: turn off `import/no-deprecated`

* fix: turn on `ts/no-shadow`

* chore: update deps

* feat: add `eslint-plugin-react-refresh`

* feat: add `eslint-plugin-command`

* chore: update vscode settings

* chore: update release action

* feat: update stylistic and ts-eslint

* feat: add rule `vue/script-indent`

* feat: turn off `style/indent` for vue files

* fix: config rule `vue/script-indent`

* fix: config style rules

* fix: config rule `vue/brace-style`

* fix: config rule `style/brace-style`

* fix: config rule `curly`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.