sveltejs / language-tools

The Svelte Language Server, and official extensions which use it
MIT License
1.26k stars 200 forks source link

Better type safety for two way binding #1392

Open probablykasper opened 2 years ago

probablykasper commented 2 years ago

Describe the bug

There's no svelte-check error when binding to a property if the property is a broader type.

To Reproduce

<script lang="ts">
  import MyComponent from './MyComponent.svelte'
  let value: Date
  $: console.log('value:', value)
</script>

<MyComponent bind:value />

MyComponent.svelte:

<script lang="ts">
  export let value: Date | null = null
</script>

{JSON.stringify(value)}

Expected behavior

There should be a type error because value is defined as Date even though it can also be null

System

dummdidumm commented 2 years ago

This works as expected. Date | null means "you can pass one of the two types in here", so Date is correct. If it's about JSON.stringify not throwing an error: you probably didn't turn on strict mode in your tsconfig.

dummdidumm commented 2 years ago

Ah wait, do you mean that, because it's a binding, this should check both ways, i.e. that Date | null is also assignable to Date?

probablykasper commented 2 years ago

Yes. Even though we define value as Date, the value ends up being null

dummdidumm commented 2 years ago

Implementation notes:

ptrxyz commented 2 months ago

Will this still be a problem with Svelte 5? It's a bit dangerously to have the type checker being fine and then .... bam! ... Things break. 😬

frederikhors commented 2 months ago

Will this still be a problem with Svelte 5? It's a bit dangerously to have the type checker being fine and then .... bam! ... Things break. 😬

Yeah, it's annoying but really helpful because I had hundreds of possible errors that I'm fixing right now thanks to svelte-check@4.

dummdidumm commented 2 months ago

Out of curiosity, since you said "hundreds of possible errors" - can you give an example of one such error? Hundreds sounds a bit much, so I just want to make sure that we didn't accidentally create false positives.

frederikhors commented 2 months ago

Out of curiosity, since you said "hundreds of possible errors" - can you give an example of one such error? Hundreds sounds a bit much, so I just want to make sure that we didn't accidentally create false positives.

Not at all. My code was the problem.

Hundreds because my errors were in many components.

90% the bind:value issue with a type string | null in cmp and only string when I was calling the component.

The null is needed because many fields may come as null from the server.

dummdidumm commented 1 month ago

Unfortunately #2477 didn't work out, we had to revert it, therefore reopening. See #2508 for more details

Malix-Labs commented 1 month ago

I don't see #2477 as reverted

huntabyte commented 1 month ago

I don't see #2477 as reverted

It's merged it's just pending a release @Malix-Labs