Skip to content

Conversation

@dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Apr 21, 2021

This changes the generation for accessing the events/slots of a component. Instead of using instanceOf, the component instance is created using the constructor and passing the props. This enables TypeScript to infer generic relationships between props and slots/events.
#945
Note that this change does not cover all cases: If someone forwards a slot or an event whose type is generic and needs to be infered, that type will remain unknown, because reinstantiating the context in the return { props: .. } step is really hard.

TODO:
How to fix the "varialbe used before instantiation"-error that occurs when we tranform <Foo prop={prop} let:prop>? The transformation for let:prop is const prop = new Foo({.. props: {prop: prop}}) which TS doesn't like. @jasonlyu123 any ideas?

This changes the generation for accessing the events/slots of a component. Instead of using `instanceOf`, the component instance is created using the constructor and passing the props. This enables TypeScript to infer generic relationships between props and slots/events.
sveltejs#945
Note that this change does not cover all cases: If someone forwards a slot or an event whose type is generic and needs to be infered, that type will remain unknown, because reinstantiating the context in the `return { props: .. }` step is really hard.
@dummdidumm dummdidumm marked this pull request as draft April 21, 2021 07:38
@jasonlyu123
Copy link
Member

The only solution I can think of now is to declare a new variable. Something similar to if-scope or pass it as a parameter to an immediately invoked function.

const __propsArgs = { a }
() =>{  
  const a = new Component({ props: __propsArgs  }).$$slotdef['default']
}

or

((__propsArgs) =>{  
  const a = new Component({ props: __propsArgs  }).$$slotdef['default']
})({ a })

@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 21, 2021

Yeah sounds like the only option. So there needs to be some logic to track which props there are both on the default and the names slots, and if there's one that's duplicated, construct a immediately invoked function from the default slot and reuse that in all the inner slots.
Edit: Just had the idea, we could reuse the ifScope-redeclare-for-control-flow-logic here.

@dummdidumm dummdidumm marked this pull request as ready for review April 22, 2021 14:48
@dummdidumm
Copy link
Member Author

Ok I reused the ifScope logic here, worked out well, although I still feel that this code is getting more and more complex - if it's just inevitable or I'm writing shitty code, can't tell for sure right now 😄 @jasonlyu123 could you have a look?

Copy link
Member

@jasonlyu123 jasonlyu123 left a comment

Choose a reason for hiding this comment

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

Mostly working great. Found a small problem when an attribute is empty. And some minor issues to discuss.

@dummdidumm
Copy link
Member Author

These were great suggestions, thanks! Especially that string template comment was a moment of enlightment 😄

@dummdidumm dummdidumm merged commit ef1d791 into sveltejs:master Apr 23, 2021
@dummdidumm dummdidumm deleted the slots-events-infer-from-props branch April 23, 2021 07:40
@ryu-man
Copy link

ryu-man commented Apr 23, 2021

I noticed that a string value is inferred as "any" type in the slot prop!

@dummdidumm
Copy link
Member Author

Should be fixed with the latest release

@ryu-man
Copy link

ryu-man commented Apr 23, 2021

I am using the latest release, all values get inferred normally, but when using a string value it inferred as "any" type not "string" type

@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 24, 2021

Can't reproduce. What's your version? Latest is 104.10.1 . If the issue persists with the latest version after restarting VS Code, please open a separate issue with detailed instructions.

@ryu-man
Copy link

ryu-man commented Apr 24, 2021

Great, I tried that one more time and it's working fine now.

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.

3 participants