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

Better type safety for two way binding #1392

Open
probablykasper opened this issue Feb 19, 2022 · 11 comments
Open

Better type safety for two way binding #1392

probablykasper opened this issue Feb 19, 2022 · 11 comments
Labels
bug Something isn't working

Comments

@probablykasper
Copy link

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

  • OS: macOS
  • IDE: VSCode
  • Plugin/Package: "Svelte for VSCode", svelte-check, svelte2tsx
@probablykasper probablykasper added the bug Something isn't working label Feb 19, 2022
@dummdidumm
Copy link
Member

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

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?

@dummdidumm dummdidumm reopened this Feb 19, 2022
@probablykasper
Copy link
Author

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

@dummdidumm
Copy link
Member

Implementation notes:

  • only do this for the new transformation
  • We need to assign both ways: the variable as an input to the component, the component instance's prop to the variable. Since we can't assign the component to the property since we assign the property to the component during its construction, we also can't have built-in mappings and therefore need to transfer the error in one case to the correct position somehow - maybe through another comment pragma. Code could look something like this: const component = new Component({..., props: {...binding: variable});/* mapMeToX */variable = component.$$prop_def.binding/* endMapMeToX */

@dummdidumm dummdidumm changed the title No type error when binding to different type Better type safety for two way binding Mar 23, 2022
@ptrxyz
Copy link

ptrxyz commented Aug 27, 2024

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. 😬

@dummdidumm dummdidumm added the Fixed Fixed in master branch. Pending production release. label Aug 28, 2024
dummdidumm added a commit that referenced this issue Aug 28, 2024
#1392

This adds enhanced type checking for bindings in Svelte 5: We're not only passing the variable in, we're also assigning the value of the component property back to the variable. That way, we can catch type errors like the child binding having a wider type as the input we give it.
It's done for Svelte 5 only because it's a) easier there b) doesn't break as much code (people who upgrade can then also upgrade the types, or use type assertions in the template, which is only possible in Svelte 5).
There's one limitation: Because of how the transformation works, we cannot infer generics. In other words, we will not catch type errors for bindings that rely on a generic type. The combination of generics + bindings is probably rare enough that this is okay, and we can revisit this later to try to find a way to make it work, if it comes up.
Also note that this does not affect DOM element bindings like <input bind:value={...} />, this is only about component bindings.
@frederikhors
Copy link
Contributor

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

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

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 dummdidumm removed the Fixed Fixed in master branch. Pending production release. label Sep 19, 2024
@dummdidumm dummdidumm reopened this Sep 19, 2024
@dummdidumm
Copy link
Member

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

dummdidumm added a commit that referenced this issue Sep 19, 2024
This reverts commit 8c080cf.

This reverts #2477. Sadly, the idea didn't work out, as shown by two opened bug reports:

- #2506: A type union can be narrowed on the input, but not on the way back out
- #2494: A generic nested within a bound value is not properly resolved and not falling back to `any` (in #2477 we thought of the generic case, but only for when the generic is the whole value type, not when it's nested)

For these reasons I don't see a way to properly implement #1392 at the moment.
@Malix-Labs
Copy link

I don't see #2477 as reverted

@huntabyte
Copy link
Member

I don't see #2477 as reverted

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants