fix: remove async support from effectFn and cleanupFn

[dmaskasky]

Remove async support from effectFn and cleanupFn.

Related Discussion: #10
Related PR: pmndrs/jotai#2189

TODO: bump minor after this change.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6b99ea8:

Sandbox	Source
React	Configuration
React TypeScript	Configuration

[dai-shi]

Looks good to me. This is so much more predictable.

[dmaskasky]

Looks good to me. This is so much more predictable.

I agree. The main issue with async I feel is the expectation that the effectFn will rerun multiple times independently but in reality atom deps are not updated until the promise resolves and overlapping runs will block each other.

[dmaskasky]

We wouldn’t want to set atom values synchronous to the read evaluation while the store is recomputing dependencies. But I think it should be fine in the next microtask after this recomputation has completed.

…

On Mon, Oct 16, 2023 at 5:58 PM Daishi Kato ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/atomEffect.ts <#12 (comment)>: > @@ -49,8 +48,9 @@ export function atomEffect( } ++ref.inProgress try { - await ref.cleanup?.() - ref.cleanup = await effectFn(get, setSelf as Setter) + await void 0 Yes, what I mean is if we *want* to warn such usage, because it's usually a bad idea in Jotai way. — Reply to this email directly, view it on GitHub <#12 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGNG6JHB63BKIHXVCKYKOTX7XJZ5AVCNFSM6AAAAAA6C7AQZWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMOBRGE2TENRXHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>