sveltejs / svelte

web development for the rest of us
https://svelte.dev
MIT License
79.7k stars 4.23k forks source link

Svelte5 5: VSCode Migrator adding $state rune to bind:this variables #13904

Open KieranP opened 1 week ago

KieranP commented 1 week ago

Describe the bug

Have component that binds textarea to variable for use on afterMount

<script lang="ts">
  import { onMount } from 'svelte'

  export let autofocus = false

  let textarea: HTMLTextAreaElement

  onMount(() => {
    if (autofocus) {
      textarea.focus()
    }
  })
</script>

<textarea bind:this={textarea}></textarea>

This is being converted into this:

<script lang="ts">
  import { onMount } from 'svelte'

  interface Props {
    autofocus?: boolean;
  }

  let { autofocus = false }: Props = $props();

  let textarea: HTMLTextAreaElement = $state()

  onMount(() => {
    if (autofocus) {
      textarea.focus()
    }
  })
</script>

<textarea bind:this={textarea}></textarea>

But defining let textarea is throwing this typescript error:

Type 'HTMLTextAreaElement | undefined' is not assignable to type 'HTMLTextAreaElement'.
  Type 'undefined' is not assignable to type 'HTMLTextAreaElement'.

Manually changing let textarea: HTMLTextAreaElement = $state() to just let textarea: HTMLTextAreaElement fixes the issue.

Reproduction

See above

Logs

No response

System Info

System:
    OS: macOS 15.0.1
    CPU: (12) arm64 Apple M3 Pro
    Memory: 259.27 MB / 18.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.15.1 - ~/.asdf/installs/nodejs/20.15.1/bin/node
    Yarn: 4.3.1 - ~/.asdf/installs/nodejs/20.15.1/bin/yarn
    npm: 10.8.2 - ~/.asdf/plugins/nodejs/shims/npm
    bun: 1.1.0 - ~/.bun/bin/bun
  Browsers:
    Chrome: 130.0.6723.70
    Safari: 18.0.1
  npmPackages:
    svelte: ^5.1.0 => 5.1.0

Severity

annoyance

nmzein commented 1 week ago

It should be converting it to let textarea: HTMLTextAreaElement | undefined = $state() but this seems to have been skipped because before migration it already had a type.

KieranP commented 1 week ago

If bind:this is run when the component is created through, why does the variable it binds to need to be reactive? It won't change, it's assigned once... seems like Svelte 5 is requiring things be $state runes when perhaps they don't need to be?

brunnerh commented 1 week ago

The rules around bind:this have already been loosened before, otherwise you would get this warning without $state:

textarea is updated, but is not declared with $state(...). Changing its value will not correctly trigger updates (non_reactive_update)

Migration should probably add | undefined to the type if a variable without initialization is turned into $state (unless undefined is removed from the $state type).

A question is whether $state should be added for this bindings like these. Both the addition of | undefined and making it a $state is more correct, the variable does start out as undefined and is only set during mount.

Code like the following would throw (which is not detected by TS):

let textarea: HTMLTextAreaElement;

log();

function log() {
  console.log(textarea.textContent);
}
seantiz commented 6 days ago

I ran into the same error tonight. The only difference was mine was a wider HTMLElement, but otherwise exactly the same behaviour.