Skip to content

feat: allow as expressions for bindable props#2372

Merged
dummdidumm merged 5 commits into
sveltejs:masterfrom
paoloricciuti:allow-as-expression-in-$props
May 17, 2024
Merged

feat: allow as expressions for bindable props#2372
dummdidumm merged 5 commits into
sveltejs:masterfrom
paoloricciuti:allow-as-expression-in-$props

Conversation

@paoloricciuti

Copy link
Copy Markdown
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
Copy Markdown
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

jasonlyu123 commented May 17, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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

@dummdidumm

Copy link
Copy Markdown
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
Copy Markdown
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

@dummdidumm dummdidumm left a comment

Copy link
Copy Markdown
Member

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
@brianschwabauer

Copy link
Copy Markdown

@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