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

(rework) extend from svelte2tsx base class #386

Merged

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Jul 30, 2020

Things like $$prop_def live in a Svelte2TsxComponent base class now.
Saves some characters per transformation and opens up the door for easier manual d.ts creation.

Would now look something like this:

// ...
function render() {
// ...
}
const Input__SvelteComponent_ = createSvelte2TsxComponent(render);

export default class Input__SvelteComponent_  extends _Input__SvelteComponent_ {
}

Thoughts @orta @halfnelson @jasonlyu123 ?

TODO

  • adjust tests
  • all props optional in js/ts-non-strict mode

Things like `$$prop_def` live in a `Svelte2TsxComponent` base class now.
Saves some characters per transformation and opens up the door for easier manual `d.ts` creation.
@dummdidumm
Copy link
Member Author

dummdidumm commented Jul 31, 2020

Thinking about generic props and defining dependencies between props/slots/events, maybe it's better to have one big generic with all parts instead. Also, in order to get type inference, we no longer could use classes - but we also cannot really use functions because of how the real svelte components are typed (they are classes).. In order to have them interact with each other, we would need to place events and slots within the props property, because of jsx only letting us define a single props property through which all typing is done (I think ...).

@dummdidumm
Copy link
Member Author

The problem with generics is hard. Slots/events are generated something like __sveltets_instanceof(Component) - how to get the generic in there?

@halfnelson
Copy link
Contributor

halfnelson commented Aug 1, 2020 via email

@dummdidumm
Copy link
Member Author

The use case for generics would be to be able to express something like this:

props: {items: T[]}, slots: {item: T}

So that this would give an error

<Component items={[1,2]} let:item={item}>
   {item.toLowerCase()} // <--- error, is number, not string
</Component>

@halfnelson
Copy link
Contributor

I don't see a clean way of doing this. I think the best we can do is support typescript expressions in the template,
The component would just type items as unknown[] and the type of item would be unknown and would force the caller to cast, the caller would know the type of item at that stage (or could use a helper type to determine it typeof items extends readonly (infer T)[] ? T : never,

@@ -94,3 +97,14 @@ declare function __sveltets_each<T>(
array: ArrayLike<T>,
callbackfn: (value: T, index: number) => any
): any;

declare class Svelte2TsxComponent<Props = {}, Slots = {}, Events = {}> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be ideal for this to also extend SvelteComponent, maybe this can be an interface instead?
We want instanceOf checks to work instead of returning ("This can never be true"), it would also make our exported .d.ts files generated from this compatible with the svelte component api
(I know the old code didn't do this, but that was due to type conflicts I was having with svelte @types, I think this was resolved)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah was thinking the same, we should extend from the base class

Copy link
Member Author

@dummdidumm dummdidumm Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhm, we cannot extend from it, because we need to redeclare the component ourselves at the top. But we can at least copy the interface and constructor, so they are structurally (almost) the same (except for $$prop_def / $$slot_def).

@dummdidumm
Copy link
Member Author

dummdidumm commented Aug 2, 2020

The generics work if they are only within the props, so TS can infer that. Maybe there is a way to infer the type (haven't testet it yet) for generics between props and slots/events: instead of doing instanceof(Component), we do new Component({props:{<all props that the user passed in>}}), and maybe then TS can infer the types for slots/events. This would be enough because you cannot define generics that do not relate to props.

I think adding TS support to the template will not help because of the way svelte2tsx currently generates code for slots/events. Explicitly typing things will not help when people want to express dependencies between props and slots/events.

@dummdidumm
Copy link
Member Author

dummdidumm commented Aug 3, 2020

I confirmed generics will work if we change the generation for slots/events from __instanceOf(Component) to new Component({props: {...props on that component}}). TypeScript can then from the constructor invocation infer the generics, if the component definition is like this:

declare class Svelte2TsxComponent<Props = {}, Slots = {}, Events = {}> {
    constructor(options: {
        target: Element;
        anchor?: Element;
        props?: Props; // <-- this is where the magic happens
        hydrate?: boolean;
        intro?: boolean;
        $$inline?: boolean;
    });
    $$prop_def: Props;
    $$slot_def: Slots;
    $on: SvelteOnAllEvent<Events>;
    $destroy(): void;
    $set(props: Partial<Props>): void;
}

I will leave this out of this PR though because we first need to agree on a way to define the generics inside a Svelte file.

@dummdidumm
Copy link
Member Author

dummdidumm commented Aug 3, 2020

I updated the typings. I switched slots/events because I think events are more likely to be used/typed than slots, so slots is now last. The type error at the top of the shim file can be ignored (I wonder why it did work with the export default class syntax before, because you cannot merge default export declarations according to the TS docs).

@jasonlyu123 @halfnelson could you have one more look if this makes sense to you? The new Svelte2TsxComponent class could in a second step be exported from svelte2tsx so that people can use/extend the class in their typings like this:

// index.d.ts definition file:
import { SvelteComponent } from 'svelte2tsx';

export class MyPublicComponent extends SvelteComponent<{propA: boolean}, {eventB: CustomEvent<boolean>}, {slotC: boolean}> {}

I briefly thought about asking to add the component type definition to the main repo, but I'm not sure they would accept it due to the $$prop_def / $$slot_def things.

@jasonlyu123
Copy link
Member

Overall ok with the direction 😀. Some suggestion/idea:

  1. Maybe the definition of $on method can also be moved to Svelte2TsxComponent so that it won't be property anymore but I haven't found a way to change the __sveltets_bubbleEventDef.
  2. if we export the base type of SvelteComponent in svelte2tsx, or even a new module, we can also export the generic constraint.
  3. Add some warning with docstring on $$props_def and $$slot_def. we can't make it disappear if used in the ts/js files at least add some warning

@dummdidumm
Copy link
Member Author

  1. Maybe the definition of $on method can also be moved to Svelte2TsxComponent so that it won't be property anymore but I haven't found a way to change the __sveltets_bubbleEventDef.

Not sure what you mean, Svelte2TsxComponent contains the $on method.

  1. if we export the base type of SvelteComponent in svelte2tsx, or even a new module, we can also export the generic constraint.

To clarify: The Props, Events, Slots generics? Yes we should definitely do that. I think the safest way would be to just copy-paste the content of Svelte2TsxComponent and export that.

  1. Add some warning with docstring on $$props_def and $$slot_def. we can't make it disappear if used in the ts/js files at least add some warning

Good idea! Will do.

@jasonlyu123
Copy link
Member

jasonlyu123 commented Aug 3, 2020

Not sure what you mean, Svelte2TsxComponent contains the $on method.

Make it a method instead of a property but this is no very important.

@dummdidumm
Copy link
Member Author

Not sure what you mean, Svelte2TsxComponent contains the $on method.
Just make it a method instead of a property but this is no very important.

Ah I understand. I honestly don't know how to rework this into a method and preserve proper typing 😄 So I'll leave it as a property which is a function now.

Simon Holthausen added 3 commits August 4, 2020 15:38
- add partial/with_any logic
- make output a little smaller
- adjust tests
Now typed as a method. New optional generic `StrictEvents` to disallow any other events than the typed ones.
@dummdidumm
Copy link
Member Author

@jasonlyu123 I found a way to type the $on-method, and also enhanced the class definition so that devs could specify that any events other than the typed ones will throw a type error. Could you take one more look?

@AlexGalays
Copy link

fwiw, I used generics extensively in the past, to make safer component API.

An example would be a dropdown/comboBox component with the following, simplified interface (in React):

interface ComboBoxProps<ITEM> {
  items: Array<ITEM>;
  selectedItem?: ITEM;
  itemRenderer: (item: ITEM) => VNode;
  onSelectItem: (item: ITEM) => void;
}

@dummdidumm
Copy link
Member Author

Something like that will be possible with the new class.

Definition:

class Combobox<ITEM> extends Svelte2TsxComponent<{items: ITEM[]}, {selectItem: CustomEvent<ITEM>}> {}

Usage:

<script lang="ts">
   let items = ['a', 'b'];
</script>

<Combobox items={items} on:selectItem={item => {/*item is of type string*/} />

Getting the ITEM generic to resolve correctly at runtime and exposing the Svelte2TsxComponent publicly will be part of following PRs though.

@dummdidumm dummdidumm merged commit a032ff2 into sveltejs:master Aug 6, 2020
@dummdidumm dummdidumm deleted the rework-svelte2tsx-class-generation branch August 6, 2020 07:52
@jasonlyu123
Copy link
Member

jasonlyu123 commented Aug 6, 2020

I found the event forward from the component is not working anymore. But upon further investigation, I found my original implementation doesn't work properly either lol and my original way is even more problematic. I'll create another PR if I somehow find a way to do it.

@dummdidumm
Copy link
Member Author

What exactly does not work? I tested with

Child:

<script lang="ts">
  export let aProp: string;
</script>

<slot aSlot="{aProp}" />

<button on:click></button>

Parent:

<script>
  import Child from './Child.svelte';
</script>

<Child aProp="{''}" let:aSlot="{joo}" on:click="{(e) => e}">{joo}</Child>

And props/slots/event was typed correctly

@jasonlyu123
Copy link
Member

jasonlyu123 commented Aug 6, 2020

<script>
  import Child from './Child.svelte';
</script>

<Child on:click aProp="{''}" let:aSlot="{joo}" on:click="{(e) => e}">{joo}</Child>

Ancestor

<script>
  import Parent from './Parent.svelte';
</script>

<Parent aProp="{''}" let:aSlot="{joo}" on:click="{(e) => e}">{joo} />

I originally want this on:click to also has MouseEvent but it actually makes any event MouseEvent.

@dummdidumm
Copy link
Member Author

Aaah I get it now, thanks for explanation. I'll also have a look later hopefully at this.

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.

4 participants