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

Add async functions to the $effect parameter type #10498

Closed
Thiagolino8 opened this issue Feb 16, 2024 · 10 comments
Closed

Add async functions to the $effect parameter type #10498

Thiagolino8 opened this issue Feb 16, 2024 · 10 comments

Comments

@Thiagolino8
Copy link

Describe the problem

The onMount function typing allows the reception of an asynchronous function as long as this function does not return a cleanup
The typing of the $effect rune does not allow the same, even though it is much more useful for it than for onMount and even though $effect is being marketed as an alternative to onMount

Describe the proposed solution

Change the typing to, just like onMount, allow asynchronous function as a callback as long as this callback does not return a cleanup

Importance

nice to have

@Conduitry
Copy link
Member

Unlike onMount where the issue is just that it won't correctly call the cleanup function if it's returned asynchronously, the issue with $effect is that it won't correctly mark as dependencies anything that is accessed by the function asynchronously after it's run. So I think this would be a misleading type change to make.

@Thiagolino8
Copy link
Author

This problem only occurs in asynchronous functions in dependencies that are read after an await, and the same problem exists in non-asynchronous functions in dependencies that are not read immediately on the first execution

Callbacks, if blocks, try catch, ternaries, switch statements, labels, all of these can cause the same problem

I believe the problem is the opposite, not adding typing for asynchronous functions implies that they do not work regardless of use, which is not true

The only solution to this problem is good documentation

@brunnerh
Copy link
Member

Callbacks, if blocks, try catch, ternaries, switch statements, labels, all of these can cause the same problem

For most of those it's a feature. The effect only reruns if its actual dependencies change.
For async stuff, things that are dependencies are not necessarily recognized at all.

Related:

@Skaldebane
Copy link

Skaldebane commented Feb 16, 2024

Came here to say the same thing!

I was surprised while migrating some Svelte 4 code from onMount to $effect that async isn't supported.

Reading through previous responses, I can see the issues with that, but I think it should be possible if the compiler knows that we've untracked all state variables, or maybe with a separate function ($effect.mount maybe?)

I don't know much about the internals, but these are my 2c :p

Edit: I just realized that it works just fine if I ignore TypeScript errors, so it's just a typing issue.

@trueadm
Copy link
Contributor

trueadm commented Feb 23, 2024

Effects can't take async functions, so closing this issue.

@trueadm trueadm closed this as completed Feb 23, 2024
@Thiagolino8
Copy link
Author

Thiagolino8 commented Feb 23, 2024

Can't or shouldn't?
Because it works perfectly as long as the dependency is not read after an await

@Thiagolino8
Copy link
Author

And as already mentioned, one of the uses of $effect is to be an alternative to onMount
If onMount can receive asynchronous functions, then $effect can also receive them

@trueadm
Copy link
Contributor

trueadm commented Feb 23, 2024

@Thiagolino8 $effect isn't the same as onMount because onMount doesn't track anything whilst effects do. If you want an async function then you need to call it from the function inside your effect instead.

Maybe we should change the documentation around this as I think we can keep onMount around.

@dummdidumm
Copy link
Member

There's one thing we could do: allow async functions, but then they cannot return anything because people might return something expecting the cleanup function to be called. I'm not sure it's worth it though

@trueadm
Copy link
Contributor

trueadm commented Feb 23, 2024

@dummdidumm I thought about that but if you have an async function, it's so you can await and then reactive tracking will completely fail, so it's just not worth it. Other frameworks throw an error when you try and do this with effects, so maybe we should consider that too.

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

No branches or pull requests

6 participants