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

feat: allow as expressions for bindable props #2372

Merged

Conversation

paoloricciuti
Copy link
Member

As per discussion on discord this change allow for declaring the type of a bindable prop using the as type expression

eg.

<!-- Parent Component -->
<script lang=ts>
  import Child from './Child.svelte';
  let amount = $state(10);
</script>
<Child bind:amount />

<!-- Child Component -->
<script lang=ts>
  import Child from './Child.svelte';
  let {
    amount = $bindable(0) as number,
  } = $props();
</script>
{amount}

i would just need a bit of guidance to determine where to write the test.

@paoloricciuti
Copy link
Member Author

@dummdidumm can i ask you where i should put the test here?

P.s. if you can give a general rule of thumb on how to determine where to put them it would be great for the next times too 😄

@jasonlyu123
Copy link
Member

jasonlyu123 commented May 17, 2024

Personally unsure about this because the type assert looks redundant. The behaviour also slightly deviates from a ts file where the amount is typed as any. But again, we did transform export let hi: string | number to eliminate the control flow narrowing. That also deviates from a ts file.

https://www.typescriptlang.org/play/?ssl=11&ssc=39&pln=1&pc=1#code/DYUwLgBA3gUBEEMC2B7ArgO0gXggEgCMBLDAEwQNAAoAGASkQGcIM0kCQAnAGhgF8IuPAAdOKYYyp0A3DBjJ0WGAHplEAHoB+OTFIgAxsAScQEAGaZ9YIigz5R4yXQBciDAE85ew8dMWMVjZ2hCTklCAAPAAqAHxUAG6uUS4QUUA

@paoloricciuti
Copy link
Member Author

Personally unsure about this because the type assert looks redundant. The behaviour also slightly deviates from a ts file where the amount is typed as any. But again, we did transform export let hi: string | number to eliminate the control flow narrowing. That also deviates from a ts file.

https://www.typescriptlang.org/play/?ssl=11&ssc=39&pln=1&pc=1#code/DYUwLgBA3gUBEEMC2B7ArgO0gXggEgCMBLDAEwQNAAoAGASkQGcIM0kCQAnAGhgF8IuPAAdOKYYyp0A3DBjJ0WGAHplEAHoB+OTFIgAxsAScQEAGaZ9YIigz5R4yXQBciDAE85ew8dMWMVjZ2hCTklCAAPAAqAHxUAG6uUS4QUUA

Is not something I would personally do but it was pointed out and I think is worth have a safeguard against those who does it.

@dummdidumm
Copy link
Member

While I agree with @jasonlyu123 that it's weird to do that, it's still valid TS syntax and we should support it so this property is properly marked as bindable. As for the test: I propose to enhance the ts-runes-bindable test case with a c = $bindable(0) as number

@paoloricciuti
Copy link
Member Author

While I agree with @jasonlyu123 that it's weird to do that, it's still valid TS syntax and we should support it so this property is properly marked as bindable. As for the test: I propose to enhance the ts-runes-bindable test case with a c = $bindable(0) as number

Added the test...for the future how can i pick between svelte2tsx and htmlx2jsx?

@paoloricciuti
Copy link
Member Author

@dummdidumm the test failing is unrelated to this change...should i update that here?

@dummdidumm
Copy link
Member

Yeah you can update it here.
How to differentiate: htmlx2jsx is for when this is purely about template transformations, svelte2tsx is for everything on top

@paoloricciuti
Copy link
Member Author

How to differentiate: htmlx2jsx is for when this is purely about template transformations, svelte2tsx is for everything on top

Oh got it! Thanks

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thank you!

@dummdidumm dummdidumm merged commit 3147c81 into sveltejs:master May 17, 2024
3 checks passed
@brianschwabauer
Copy link

@dummdidumm Can we get a new release with this in it? I appreciate the work you guys put into 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