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

Svelte 5: the new Bindable types completely destroyed my projects types. #11356

Closed
HighFunctioningSociopathSH opened this issue Apr 28, 2024 · 17 comments · Fixed by #11420
Closed
Assignees
Milestone

Comments

@HighFunctioningSociopathSH
Copy link

HighFunctioningSociopathSH commented Apr 28, 2024

Describe the bug

The new types for bindablity might work for simple types but for a complex component that uses many generics, it seems that they destroy the types. specially if other components inherit a component with generic type parameters. generics that were working fine before the update now no longer work. Honestly, I don't understand why svelte should be the one to add this type to every property without them knowing about it. The user should be the one to decide whether a property MUST be bind to or it can be bind to. Especially since Svelte's typescript with generics is so slow and inconsistent that it makes you hate library development with typescript.
Wrapping entire types in generics like WithBindable or things like that ruins generics in svelte. I had occasions where simply trying to omit a property from the result of a generic type destroys it. It's really hard for me to recreate the issue but I request a way to opt out of this, otherwise further upgrades will be impossible for me. Atleast allow the user themselves to wrap their props' type with Bindable or better, let them decide that on each property themselves like

type Test = {
  something?: Bindable<string>;
}

Reproduction

currently can't reproduce the issue in a simple and small type

Logs

No response

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (16) x64 12th Gen Intel(R) Core(TM) i7-12650H
    Memory: 4.94 GB / 15.63 GB
  Binaries:
    Node: 18.14.2 - C:\Program Files\nodejs\node.EXE
    npm: 9.7.1 - C:\Program Files\nodejs\npm.CMD
    bun: 1.1.3 - ~\.bun\bin\bun.EXE
  Browsers:
    Edge: Chromium (123.0.2420.97)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    svelte: ^5.0.0-next.115 => 5.0.0-next.115

Severity

annoyance

@dummdidumm dummdidumm added the awaiting submitter needs a reproduction, or clarification label Apr 28, 2024
@dummdidumm
Copy link
Member

We need a reproduction or else we can't really do anything here.

@HighFunctioningSociopathSH
Copy link
Author

Can't you just don't do anything instead of forcefully adding types? what is wrong with the user doing it themselves? I really don't want my type to get wrapped in another generic that I don't even know what it actually does. Right now the type assigned to props is wrapped with the WithBindable generic and that's just not something you should enforce. Make the type available and anybody who wants it can use it. All this time i was fine with adding "bind:property" to my types whenever a prop was bindable.

@dummdidumm
Copy link
Member

You didn't have to do that previously. please provide a reproduction (it's fine if it's not minimal if it's hard to shrink down), we will not consider changing any of this without understanding the problem better

@HighFunctioningSociopathSH
Copy link
Author

HighFunctioningSociopathSH commented Apr 28, 2024

@dummdidumm
I managed to reproduce the problem in a smaller scale
Test.svelte

<script lang="ts" generics="DisableResponsive extends boolean = false">
  import type { TestType } from "./type";

  let { ...restProps }: TestType<DisableResponsive> = $props();
</script>

And this is my .ts file

type Type1<DisableResponsive extends boolean> = DisableResponsive extends false
  ? { someProps?: { xlg?: boolean; lg?: boolean; sm?: boolean } }
  : { someProps?: boolean };

type Type2<DisableResponsive extends boolean> = {
  disableResponsive?: DisableResponsive;
};

export type TestType<DisableResponsive extends boolean = false> = Type1<DisableResponsive> & Type2<DisableResponsive>;

Now in version 113 everything is fine and when you try to render Test component like below you will get an error saying someProps can not take an object indicating the generic works.

<script lang="ts">
  import Test from "$components/Test/Test.svelte";
</script>

<Test disableResponsive someProps={{ lg: false }}></Test>

but after update there is a sort of crash in types where the props no longer have any types and the disableResponsive generic no longer works. In my experience, this usually happened when I wrapped generics in other generics so that made me think it's because of WithBindable generic.

Maybe the problem is caused because typescript is no longer inferring false or true from the type of disableResponsive since the type of disableResponsive is changed to Bindable<false, undefined> or Bindable<true, undefined> and that no longer extends boolean. just a guess

@dummdidumm
Copy link
Member

Have you upgraded to the latest version of the VS Code extension and svelte-check? I can't reproduce this.

@dummdidumm
Copy link
Member

Looking closer at the type, I'm pretty sure it's somewhat brittle either way and any kind of simple mapped types will make it break. You don't need a generic in this case, use a union type instead:

type TestType = { disableResponsive?: false; someProps?: { xlg?: boolean; lg?: boolean; sm?: boolean } }
  | { disableResponsive: true; someProps?: boolean }

@nuts-n-bits
Copy link

You don't need a generic in this case

You can't point to a scaled down example and say you don't need generics for such simple cases

@dummdidumm
Copy link
Member

If there is a non-scaled-down example that requires generics to be applied this specific way that breaks the types then I'd like to see it.

@HighFunctioningSociopathSH
Copy link
Author

HighFunctioningSociopathSH commented Apr 29, 2024

I updated to the newest svelte for vscode extension and as you mentioned the issue was fixed but some generics of my component still didn't work so I tried to find out why. apparently, the below codes generic breaks as soon as you make a prop bindable.
Test.svelte

<script lang="ts" generics="Element extends keyof HTMLElementTagNameMap = 'div'">
  import type { TestType } from "./type";

  let {
    // uncomment the following line to see that the generic for element no longer works
    // hello = $bindable(),
    ...restProps
  }: TestType<Element> = $props();
</script>

type.ts

import type { SvelteHTMLElements } from "svelte/elements";

export type TestType<Element extends keyof HTMLElementTagNameMap = "div"> = {
  // This `| keyof HTMLElementTagNameMap` in the line below is here to make vscode 
  // show the full list of suggestions even with a default value for Element since if you don't do this vscode only shows the 
  // default value as suggestion
 element: Element | keyof HTMLElementTagNameMap; 
  hello?: string;
} & SvelteHTMLElements[Element];

+page.svelte

<script lang="ts">
  import Test from "$components/Test/Test.svelte";
</script>

<Test element="span" onclick={(e) => {}}></Test>

In the page, simply change the element then hover over the event of onclick to see its currentTarget having changed.
But then uncomment the hello prop to witness that the generic no longer works.
unlike your previous workaround where you said "Don't use generics", I tried to show you an example where a generic is required

@dummdidumm
Copy link
Member

The reproduction is incomplete, what is the type of TestType?

@HighFunctioningSociopathSH
Copy link
Author

The reproduction is incomplete, what is the type of TestType?

Sorry. I have updated my comment to include the type.

@HighFunctioningSociopathSH
Copy link
Author

HighFunctioningSociopathSH commented Apr 29, 2024

In the case of vscode suggestions for generic props. It would be great if you could fix this bug where vscode suggests only the default value of a generic prop when one is supplied. for example the code above only the default value of Element generic which is 'div' is shown in suggestion by vscode instead of showing a suggestion list for what it extends. You have to remove the | keyof HTMLElementTagNameMap from the element property though. basically if you write it like below.

import type { SvelteHTMLElements } from "svelte/elements";

export type TestType<Element extends keyof HTMLElementTagNameMap = "div"> = {
 element: Element;
  hello?: string;
} & SvelteHTMLElements[Element];

then vscode only shows "div" as suggestion.

@HighFunctioningSociopathSH
Copy link
Author

HighFunctioningSociopathSH commented Apr 29, 2024

I updated to the newest svelte for vscode extension and as you mentioned the issue was fixed but some generics of my component still didn't work so I tried to find out why. apparently, the below codes generic breaks as soon as you make a prop bindable. Test.svelte

<script lang="ts" generics="Element extends keyof HTMLElementTagNameMap = 'div'">
  import type { TestType } from "./type";

  let {
    // uncomment the following line to see that the generic for element no longer works
    // hello = $bindable(),
    ...restProps
  }: TestType<Element> = $props();
</script>

type.ts

import type { SvelteHTMLElements } from "svelte/elements";

export type TestType<Element extends keyof HTMLElementTagNameMap = "div"> = {
  // This `| keyof HTMLElementTagNameMap` in the line below is here to make vscode 
  // show the full list of suggestions even with a default value for Element since if you don't do this vscode only shows the 
  // default value as suggestion
 element: Element | keyof HTMLElementTagNameMap; 
  hello?: string;
} & SvelteHTMLElements[Element];

+page.svelte

<script lang="ts">
  import Test from "$components/Test/Test.svelte";
</script>

<Test element="span" onclick={(e) => {}}></Test>

In the page, simply change the element then hover over the event of onclick to see its currentTarget having changed. But then uncomment the hello prop to witness that the generic no longer works. unlike your previous workaround where you said "Don't use generics", I tried to show you an example where a generic is required

After reverting back to svelte version 113 and still facing with the issue I can confirm that this problem is caused after updating to the svelte for vscode extension v108.4.0.
Even though Updating fixed the previous bugs, it caused the above bug where having a $bindable rune destroys the type of the component.

@dummdidumm dummdidumm added this to the 5.0 milestone Apr 29, 2024
@dummdidumm dummdidumm self-assigned this Apr 29, 2024
@dummdidumm dummdidumm removed the awaiting submitter needs a reproduction, or clarification label May 1, 2024
dummdidumm added a commit that referenced this issue May 1, 2024
Instead of using types that declare whether or not a type is bindable directly as part of the property, we're introducing a new for-types-only field to `SvelteComponent`: `$$bindings`, which is typed as the keys of the properties that are bindable (string by default, i.e. everything's bindable; for backwards compat). language-tools can then produce code that assigns to this property which results in an error we can display if the binding is invalid
closes #11356
dummdidumm added a commit that referenced this issue May 2, 2024
Instead of using types that declare whether or not a type is bindable directly as part of the property, we're introducing a new for-types-only field to `SvelteComponent`: `$$bindings`, which is typed as the keys of the properties that are bindable (string by default, i.e. everything's bindable; for backwards compat). language-tools can then produce code that assigns to this property which results in an error we can display if the binding is invalid
closes #11356
@dummdidumm
Copy link
Member

Should work again in the latest versions of Svelte + svelte-check/the IDE extension.

@justingolden21
Copy link

This might be the wrong place, but I have a svelte 5 typescript project and I have a $bindable() prop that is used by the parent, and I get the incorrect error that it "is declared but its value is never read" from ts. Is this intended behavior or a bug?

@dummdidumm

Thanks in advance 😄

@paoloricciuti
Copy link
Member

This might be the wrong place, but I have a svelte 5 typescript project and I have a $bindable() prop that is used by the parent, and I get the incorrect error that it "is declared but its value is never read" from ts. Is this intended behavior or a bug?

@dummdidumm

Thanks in advance 😄

We need a reproduction for this

@jasonlyu123
Copy link
Member

The "unused problem" should be related to sveltejs/language-tools#2268. The problem is that TypeScript doesn't know the bound value can be used outside the component.

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 a pull request may close this issue.

6 participants