Promise support in watch

[Akkuma]

https://codesandbox.io/s/breaking-watch-ct246n

If you attempt to use async/await in watch it'll break the cleanup cycle. I added a quick test locally and fooled around with it and can avoid the issue with something along these lines:

      // If there's a cleanup, we add this to the cleanups set
      if (cleanupReturn && !(cleanupReturn instanceof Promise)) {
        cleanups.add(cleanupReturn)
      }

However, I'm not sure that's the desired behavior either as that would also mean you can't run a cleanup function using async/await without additional logic, but maybe that's ok.

Derive handles this scenario already of working with promises, so it feels a little odd that watch is the odd function out when it comes to promises. I suppose one workaround is doing a manual subscribe dance around the promises and their subscriptions.

[dai-shi]

Thanks for reporting.
I'm not sure if it's correct to allow async function. watch is a wrapper around subscribe and subscribe doesn't support it, right?
useEffect doesn't allow async function either.

Derive handles this scenario already of working with promises

Does derive handles promises? Hm, we don't have cleanup functions but underive instead, so its design is different (because we want derive to return a proxy).

[Akkuma]

Here's derive using async/await https://codesandbox.io/s/derive-with-promises-yx2fkq?file=/src/App.js and here's subscribe using async/await https://codesandbox.io/s/subscribe-with-promises-7pokmq

I think the key difference is that watch is the only function that allows for an explicit cleanup function returned from it. The code for subscribe doesn't expect or check for any return value from the callback itself. I've figured out an ok alternative to use async/await with watch, by deferring async/await to another function something like:

watch((get) => {
  const asyncThing = get(state).asyncThing
  const asyncStuff = get(state2).asyncStuff
  
  doAsyncAwait(asyncThing, asyncStuff)
}

Whether watch should support async/await inside of it or if this approach should be apart of the wiki I'll leave for you to decide.

[dai-shi]

https://codesandbox.io/s/derive-with-promises-yx2fkq?file=/src/App.js
This is interesting. Thanks for sharing. It's valid because valtio supports promise values.

https://codesandbox.io/s/subscribe-with-promises-7pokmq
Okay, right. subscribe callback returns void. So, it's fine.

watch callback should return cleanup or void.
I guess it's because watch does subscribe/unsubscribe automatically. #149 @lxsmnsyc ?
And, cleanup function must not be a promise.

So, option 1 is to leave it as is. Option 2 could be removing cleanup entirely to match with subscribe, but it's a breaking change, which should be avoided if possible.

I'd be appreciated if you work on docs/wikis.

[Akkuma]

I think option 1 is superior to option 2. I've found a very nice use for watch's cleanup. You can implement really simple abort controller support on top of the cleanup. If you were to use derive, which is closer to watch than subscribe, you'd have to do arguably a not as clean approach.

watch((get) => {
  const val = get(state).val
  const ac = new AbortController()
  const run = async () => {
     const res = await fetch(val, { signal: ac.signal })
     const json = await json()
   }
   
   run()
   return () => ac.abort()
}

let ac = new AbortController()
derive({
  prop: async (get) => {
    const val = get(state).val
    ac.abort()
    ac = new AbortController
    try {
      const res = await fetch(val, { signal: ac.signal })
      const json = await json()
    } catch (e) {
      return undefined
    }
    return json.val
  }
})

The one alternative option is having specific logic to allow async/await for watch and treat an async function as if it were returning void.

[dai-shi]

The one alternative option is having specific logic to allow async/await for watch and treat an async function as if it were returning void.

I thought this would be confusing and people might misuse.
But, implementation-wise, it's fairly harmless. So, I think we can accept your suggestion.

Here's the diff that could be acceptable:

diff --git a/src/utils/watch.ts b/src/utils/watch.ts
index 14fbd63..bc41d30 100644
--- a/src/utils/watch.ts
+++ b/src/utils/watch.ts
@@ -2,7 +2,7 @@ import { subscribe } from '../vanilla'
 
 type Cleanup = () => void
 type WatchGet = <T extends object>(proxyObject: T) => T
-type WatchCallback = (get: WatchGet) => Cleanup | void | undefined
+type WatchCallback = (get: WatchGet) => Cleanup | void | Promise<void>
 type WatchOptions = {
   sync?: boolean
 }
@@ -75,7 +75,7 @@ export function watch(
       })
 
       // If there's a cleanup, we add this to the cleanups set
-      if (cleanupReturn) {
+      if (typeof cleanupReturn === 'function') {
         cleanups.add(cleanupReturn)
       }
     } finally {

Note: I dropped undefined type as it seems unnecessary. https://tsplay.dev/m3Pj1W

Would you like to open a PR with this fix and a test to cover it?

[vincerubinetti]

Just a note here, Vue allows async watch functions. Though it also doesn't utilize any kind of watch "cleanup". There are definitely good use cases.

[NeriRos]

Hey I would like to help.
Should I try to fix this and open a pull request for a review?

this is my first open source contribution :)

[dai-shi]

@NeriRos Yes, please go ahead. Note that I'm not yet confident if the outcome is mergeable.