sveltejs / svelte

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

Svelte 5: onMount accepts async function, $effect does not #13916

Open KieranP opened 1 week ago

KieranP commented 1 week ago

Describe the bug

In most cases, I'm trying to replace onMount with $effect, which seems to be the better way to go. However, $effect doesn't allow an async function:

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

  interface Props {
    theme: string
    children?: Snippet
  }

  let { theme, children }: Props = $props()

  $effect(async () => {
    await import(`@styles/themes/${theme}.scss`)
  })
</script>

{@render children?.()}

Tying Error:

 Argument of type '() => Promise<void>' is not assignable to parameter of type '() => void | (() => void)'.
  Type 'Promise<void>' is not assignable to type 'void | (() => void)'.ts(2345)

Reproduction

See above

Logs

No response

System Info

System:
    OS: macOS 15.0.1
    CPU: (12) arm64 Apple M3 Pro
    Memory: 76.77 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.2 => 5.1.2

Severity

annoyance

adiguba commented 1 week ago

Hello,

Why did you replace onMount() by $effect() ?

Why did you delay the loading of the CSS ?

Neptunium1129 commented 1 week ago

Hello,

Why did you replace onMount() by $effect() ?

Why did you delay the loading of the CSS ?

It seems that onMount is not going to be deprecated later

KieranP commented 1 week ago

@adiguba Because the docs say that $effect is run when component is mounted, in otherwords, it is the same thing as onMount. And Svelte5 is encouraging the use of $effect.

As for the delayed CSS loading, the parent component has a theme selector, which when changed, sends the updated theme value to this child component, and loads the appropriate theme file.

adiguba commented 1 week ago

onMount() is only run when the component is mounted. $effect() is run when the component is mounted, AND when any of the reactive variable are changed.

I didn't see that you use a prop on your effect, but since you're not using the return value of import(), just remove the async/await :

  $effect(() => {
    import(`@styles/themes/${theme}.scss`)
  })

And if your really need the return value, use .then() (be carefull because code inside then() will not be reactive).

webJose commented 1 week ago

Although @KieranP seems to be after $effect() for the wrong reasons on this particular instance, I'm with him in that $effect() should accept async functions. I know, this was removed because it causes confusion, etc. etc. Still, I think it is an overstep from Svelte, and the confusing cases should just be documented. My humble opinion.

dummdidumm commented 1 week ago

This is a deliberate decision to discourage async within effects, because everything that is not synchronously read will not become a dependency of the effect.

Leaving this open to see how widespread this is and to see if people are aware of that gotcha but still want this - but it's unlikely we change it.

brunnerh commented 1 week ago

And Svelte5 is encouraging the use of $effect.

It's really not. There is an entire "When not to use effects" section in the docs because the potential for misuse is huge.

Arguably, onMount should also not allow async functions. The potential bugs is different, namely the cleanup function will not work.

onMount(async () => {
  // ...

  return () => clearInterval(handle); // does nothing
})

Maybe there is a way to just prevent this scenario via types, though?

webJose commented 1 week ago

If I remember correctly, back in the day when the types for $effect allowed async functions, the documentation stated very clearly what was tracked: "and up to the first await.", I think it said. I understood well.

Anyway, you guys decide on this one. I was happy with async, now I'm not as happy, but maybe it's just me.

nmzein commented 1 week ago

Can't you do something like this:

import { untrack } from 'svelte';

$effect(() => {
    untrack(async () => {
        await fetch("/");
    })
});

I feel like when I'm thinking of converting onMount to $effect I think of it as converting to $effect with an untrack not just $effect. It is a bit clunky but maybe we can have an $effect.once to replace onMount and it would be syntactic sugar to the above.

adiguba commented 1 week ago

But why do you want to replace onMount() ?

nmzein commented 1 week ago

One less import and its kind of annoying that its the last lifecycle function thats really left. I find it kind of jarring to have $effect next to onMount in the same file.

brunnerh commented 1 week ago

That is not a good reason, and there is still onDestroy as well.

nmzein commented 1 week ago

Well if onMount was removed so would onDestroy no? I get that its not a good reason its just a personal preference, I'm not saying that some syntactic sugar needs to be implemented.

adiguba commented 1 week ago

onMount() and $effect() has two different usage...

Event if it's possible, I don't see any interest in a hacky code to simulate onMount with $effect... Just use onMount() directly...

KieranP commented 6 days ago

Ok, so let's consider this use case (please ignore that there are obviously other ways to it, like using .then/.catch, but it should demonstrate a valid use case people might want to accomplish). I can't use onMount because I want it to be triggered both on mount and when tab state is changed. But using await on the fetch is not possible unless the function is async. And function can't be async because effect doesn't allow it.

<script type="ts">
  let container: HtmlDivElement | undefined = $state()
  let tab = $state('dashboard')

  $effect(() => {
    const result = await fetch("/?tab=${tab}")
    container?.innerHTML = result
  })
</script>

<ul>
  <li onclick={() => { tab = 'dashboard' }}>Dashboard</li>
  <li onclick={() => { tab = 'settings' }}>Settings</li>
</ul>

<div bind:this={container}>
  Loading...
</div>
adiguba commented 6 days ago

You can simply use something like this :

async function loadTab(t) {
    const result = await fetch("/?tab=${t}")
    container?.innerHTML = result
}

$effect(() => {
    loadTab(tab);
});

But I don't like this sort of stuff, as result can be incoherent with the state in case of network lags...

I think it's way better to use a $derived() and an {#await} to handle the promise.

Ex: https://svelte.dev/playground/untitled?version=5.1.3#H4sIAAAAAAAACoVSXWvcMBD8K4saiE3D-fLq2m4LCfSh0HJtn6JAdNY6JyLLRlo7Dar_eyV_5PJQ6ItYzc7sSCN5ZkSLLGe_DCnSKNkVa5RGx_I7z-ilj70IBHxlfu77nRtRU8SOwuG_8LozhIbCGFa42qqeQAvzWHJGjrOKGwCNBJEmlEGbwxdq9Y0abzW2QQh_YDASm9CTUMKFI0GYpJuOxPGMXkrhTsdOWHmZfuCGU5jqCHrbtcph5Em0akSZNOIJG6T6lDxkH8OM8sKHdXpIZ11QCvdiamgGU5PqDJz5g9Up-EjhZJEGa8DgM3xfPBJILLpOj5hCWW08Tg7pp2qxGyhJ0rJaOQnf4oGuAc7gPcTxV3C93-_TRTstN5m4KbIlvyqesBj0nF2hFXSm1qp-Kn2ymK6hvEkDpqm62XZFptV_teHApMyjm6U_1s2qLLLZPBRSjbEg_048C_Wa9BQxgK-dkEG22-1mTk4nNCAFiWm5mv90Ck99RnxeixAxoLUr4_Zw-HaAHPwG-Ww2mtOI5uGDEf4mlpMdcLqf_gJQyFtExgIAAA==

webJose commented 6 days ago

Is not that $effect disallows async functions, is that its TypeScript has been made not to take it. If you ignore the TS error and write async code in your effect, it works.

To me, disallowing async code in effects feels like you're forcing me into declaring an extra function just for nothing. Why can't I have the more succinct syntax? This is my opinion on the matter.