sveltejs / svelte

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

Svelte 5: Type problem with async function in $effect rune #9520

Closed njezersek closed 1 year ago

njezersek commented 1 year ago

Describe the problem

When using the $effect rune with async function, TypeScript raises an 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)'.

However Svelte seems to work just fine with async function in `$effect$.

Here is a small example to demonstrate the problem:

<script lang="ts">
    let key = $state("test");
    $effect(async () => {
        let res = await fetch(`http://localhost:8080/api/${key}`);
        let json = await res.json();
        console.log(key, json);
    })
</script>

<input type="text" bind:value={key}>

Describe the proposed solution

Update the type definition to include async functions.

From:

declare function $effect(fn: () => void | (() => void)): void;

to:

declare function $effect(fn: () => void | (() => void) | Promise<void>): void;

Note: I realize that using an async function with $effect might make returning callback hard to handle. Thats why I propose updating the type declaration to only alow void async functions.

Alternatives considered

A possible workaround is to introduce another function:

<script lang="ts">
    let key = $state("test");
    $effect(() => {
        (async () => {
            let res = await fetch(`http://localhost:8080/api/${key}`);
            let json = await res.json();
            console.log(key, json);
        })()
    })
</script>

<input type="text" bind:value={key}>

Importance

would make my life easier

Conduitry commented 1 year ago

This is functioning as intended. $effect() blocks indeed do not work correctly with async functions - see https://svelte-5-preview.vercel.app/#H4sIAAAAAAAACkWQwW7DIBBEfwWhHIhaJemV2pb6B73HPVCyVKh4sWBJFSH-PYCV9IR2Zt8wkLmxDiKX58xRLcAl_1hX_srptrYhXsER1Dn6FHRThqiDXWmacSYHxLRPSGxku0iKQJz279WpnkmoyXpkFnWABZDEnuXmzLQxLyN7a8tUNmIHxoAmoeINNavb4_QA1J-yxD6DX2yEQ4Do3RVEv6rHYRXg4PyP6NGbUdoxHP_74vCdiGolj1I7q3_H_CxX-oO6GiXLPaY0fEOm-gWLv1hj4cIlhQTlq9wB9puwTz0BAAA=

Any references to variables that happen asynchronously inside the function will not correctly be picked up as dependencies that should re-run the effect. There's nothing that we can do about this currently without additional language features in JS for tracking execution context asynchronously, which is currently impossible.

We can't reasonably tell whether there are any references to variables before vs after the part of the function that executes synchronously, so we prevent code like this. If you really know what you are doing, you can suppress the TS warning or you can wrap in another function, as in your workaround.

manzt commented 9 months ago

$effect() blocks indeed do not work correctly with async functions

Makes sense but I'm curious if there is pattern for explicitly declaring deps of an async effect. Probably hoisting the behavior to an async function and calling that?

let href = $state("https://example.com");
let content = $state("");
$effect(() => {
 let current = href; // need to reference here so it is tracked?
 async function fetchData() {
    let response = await fetch(href);
    let data = await response.text();
    content = data;  
  }
  fetchData();
})

Like lifting fetchData to avoid the closure:

let href = $state("https://example.com");
let content = $state("");

async function fetchData(href) {
    let data = await response.text();
    content = data;  
}
$effect(() => {
  fetchData(href)
})

Is this the recommended way?

niemyjski commented 7 months ago

I find myself in this same boat...

I was loading data when Readables change. Now those are state runes and the docs say use effect which runs when any state changes I don't know how the compiler would ever know my intent when only some of the runes change but not all,,, because what if I only want to react to some of my runes? What's the best way to handle this scenario as use effect doesn't support async and this pattern will be in every app (e.g., react to parameter change and go fetch my data).

// There are many other parameters.
    parameters.subscribe(async () => await loadData());
    filter.subscribe(async () => await loadData());
    time.subscribe(async () => await loadData());

How would it also know that I have some parameters in the refactor coming from a svelte.ts file and not defined in the component? I don't want to add log statements to components just to make it get picked up...

niemyjski commented 7 months ago

@Rich-Harris I'm seeing a trend in SPA mode development as well as on the discord channel of lots of questions and issues around dealing with runes / $effect and async. Is there a general recommended approach that you'd recommend? I feel like this is a super common scenario and not everyone loads their data or async work in a page.ts esp for SPA.

khromov commented 6 months ago

@niemyjski Even if you are building a SPA the recommendation is to use SvelteKit which provide load functions for loading async data: https://kit.svelte.dev/docs/single-page-apps

niemyjski commented 6 months ago

There are a ton of scenarios where I don't know what data to load until runtime, what about posting form data etc..

khromov commented 6 months ago

You asked for a "general recommended" approach and SvelteKit is that. SPA is in fact in a heavy downwards trend - React is moving away from it with RSC, other meta frameworks like Remix and SvelteKit are very server focused. There are use cases for SPA, and SvelteKit supports it fully. You can post form data from a SPA but naturally you can't submit an actual <form> because that would require a serverful application, but there are workarounds like customizing use:enhance to send form data to a JSON API for example.

niemyjski commented 6 months ago

I have an existing backend written in .net, I’m never going to buy into server side rendering / js backend for existing and probably even new products… I don’t like this trend being forced onto us by a few frameworks. SvelteKit is really the only “option” and at that people use async browser apis and network/io calls where svelte $effect will need to play nicely. I’ll use page load where possible but it’s not always the correct choice. We are already seeing this in discord and app upgrades to 5.

ejsmith commented 6 months ago

You asked for a "general recommended" approach and SvelteKit is that. SPA is in fact in a heavy downwards trend - React is moving away from it with RSC, other meta frameworks like Remix and SvelteKit are very server focused. There are use cases for SPA, and SvelteKit supports it fully. You can post form data from a SPA but naturally you can't submit an actual <form> because that would require a serverful application, but there are workarounds like customizing use:enhance to send form data to a JSON API for example.

SPA is a downward trend because it was overused for normal marketing type sites and SSR is the right choice for those sites, but it is not the right choice for business apps with backends in non-JavaScript technology.

lolmaus commented 5 months ago

My intented attempt was like this:

let foo = $state();
let bar = $state();

$effect(async () => {
  const baz = await someAsyncStuff();

  // Svelte compiler cannot "see" below `await`, so the usage is not triggered.
  baz.use(foo, bar);
})

My workaround:

let foo = $state();
let bar = $state();

$effect(async () => {
  // Above the async part, obtain local references to necessary $state vars.
  // This lets the Svelte compiler know these are used for this $effect.
  const sync = { foo, bar };

  // Use the good old `.then` instead of `await` to keep TypeScript happy.
  someAsyncStuff().then(baz => {
    // Use local references to $state vars
    baz.use(sync.foo, sync.bar);
  });
});

@Rich-Harris Please consider adding this to docs! 🙏

bdmackie commented 5 months ago

Writing new code and have an 'auto update entity name' use case:

  1. Page level declares name via $state and $effect to detect changes.
let name = $state(document.name)
  1. Binds to name property on svelte components 2 levels deep that displays and edits the name.
<DocumentName bind:name />
  1. using superForms for the input in that bottom level component. Using a dedicated superforms instance for encapsulation and set to SPA mode to avoid knowledge of page-level forms. Valid updates set the bindable name prop.

  2. back up at the page level on the name-change detected by $effect, i'm calling fetch to do the update.

$effect(() => {
    // Auto-save name edits.
    if (!$state.is(document.name, name)) {
      console.log('name: ' + name)
      void fetch(
       // fetch args here...
      ).then(({ updated }) => {
        if (updated > 0) {
          document.name = name
          console.log('Document name updated: ' + name)
        } else {
          console.log('Document name NOT updated!')
        }
      })
    }
  })

Similar to the OP I got confused as to the recommended path at this point.

So based on my limited understanding of Svelte 5 thus far, I believe I have two options architecturally:

  1. Use the async code in $effect and treat it as a 'gotcha' which I think validates the OP's point (or doc update request), but expands it to say it's more common than "an SPA requirement".
  2. Tighten the connection between page-level forms and nested components that update data, avoiding the need for $state, $bindable and $effect altogether.

Option 2 seems to agree with "SPA is on a downtrend" but it feels like it's also saying "$state is on a downtrend", at least for this use case. Assuming the point of runes is a happy medium between client and server centricity, it'd be great to hear what approach is recommended, or if it's known runes has some dissonance baked in here.


UPDATE: For any other lost souls who find themselves here, my take on this having travelled a little farther:

While it's easy to forgive a newcomer to see the $state rune as a powerful encapsulation of both data and events, bubbling up to $effect has limitations and if you are hitting this use case, you are probably better following another maintainer recommendation and wrap state in a helper class/closure that also handles change actions. The context API can be used to reduce prop drilling across parent/child hierarchies if applicable. The result has a bit more boilerplate but not a lot, and leaves the codebase in a more extensible state.

jdgamble555 commented 2 weeks ago

I made a reusable resource function to handle this use case.

https://dev.to/jdgamble555/async-fetching-in-svelte-5-826

I think this should be implemented in Svelte 5 as $resource one day.

J