Repository: pmndrs/valtio (ts)

Issue:
This library doesn't seem to be glitch free?
First of all, thanks for sharing this very nice library!

I was doing some experiments, and I noticed that unlike most "push/pull functional reactive programming" libraries, this library is not glitch free, e.g. the callbacks of derived computations can be called with out-of-sync values, producing intermediate invalid results, and can be invoked multiple times per "leaf node" change, which can be inefficient in large "dataflow graphs".

Is this by design? To workaround this, one can apply ancient synchronous dataflow techniques, like giving each node in the dataflow a rank and using a priority queue to invoke the callbacks in the correct order. A bit like a parallel CPU stack.

Code example at https://stackblitz.com/edit/react-sqyzte

```
import React from "react";
import "./style.css";

import { proxy, useSnapshot } from 'valtio'

import { derive } from 'valtio/utils'

const state = proxy({ value: 0 });

// derived1 = state
const derived1 = derive({
  value: (get) => get(state).value,
})

// derived1 = derived2
const derived2 = derive({
  value: (get) => get(derived1).value,
})

// derived3 = state + derived1 - derived2 = state + derived1 - derived1 = state + 0 = state
const derived3 = derive({
  value: (get) => {
    // notice in the console that this callback is invoked TWICE per state update.
    const v0 = get(state).value;
    const v1 = get(derived1).value;
    const v2 = get(derived2).value; // notice in the console that v2 is out-of-sync in the first callback invocation
    const r = v0 + (v1 - v2);
    // notice the glitches in the console
    // when state is changed, this callback is invoked TWICE with out-of-sync values.
    // glitches make debugging really hard, and can cause invalid intermediate values like NaN, div by zero, etc...
    console.log(`v0=${v0} v1=${v1} v2=${v2} => r=${r}`);
    return r;
  }
})


export default function App() {
  const snap = useSnapshot(derived3)
  return (
    <div>
      <code>{snap.value}</code>
      <br/>
      <button onClick={() => ++state.value}>+1</button>
    </div>
  );
}
```



Pull request:
fix(utils): make derive glitch free
@mweststrate hinted that a solution to make glitch free is simply to use a counter. https://twitter.com/mweststrate/status/1484928361084379136

So I tried. closes #296 

I had to make the subscription sync and then async in derive. There might be some edge cases this won't work well, like mixing sync and async, but the basic patterns should be covered. Let's see how it goes with releasing this.

Patch:
diff --git a/babel.config.js b/babel.config.js
index 5029a18..fa15c8a 100644
--- a/babel.config.js
+++ b/babel.config.js
@@ -22,6 +22,7 @@ module.exports = (api, targets) => {
           runtime: 'automatic',
         },
       ],
+      ['@babel/plugin-transform-typescript', { isTSX: true }],
     ],
   }
 }
diff --git a/package.json b/package.json
index fac034d..722b5ea 100644
--- a/package.json
+++ b/package.json
@@ -135,6 +135,7 @@
     "@babel/core": "^7.16.7",
     "@babel/helper-module-imports": "^7.16.7",
     "@babel/plugin-transform-react-jsx": "^7.16.7",
+    "@babel/plugin-transform-typescript": "^7.16.8",
     "@babel/preset-env": "^7.16.8",
     "@babel/types": "^7.16.8",
     "@rollup/plugin-alias": "^3.1.9",
diff --git a/readme.md b/readme.md
index a34d14e..e5c2eb9 100644
--- a/readme.md
+++ b/readme.md
@@ -295,7 +295,7 @@ derive({
 #### `proxyWithComputed` util
 
 You can have computed values with dependency tracking with property access.
-Dependency tracking in `proxyWithComputed` conflicts with the work in `useSnapshot`.
+Dependency tracking in `proxyWithComputed` overlaps the work in `useSnapshot`.
 React users should prefer using `derive`.
 `proxyWithComputed` works well for some edge cases and for vanilla-js users.
 
diff --git a/rollup.config.js b/rollup.config.js
index b302833..450dbfa 100644
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -81,7 +81,6 @@ function createCommonJSConfig(input, output) {
         },
       }),
       resolve({ extensions }),
-      typescript(),
       babelPlugin(getBabelOptions({ ie: 11 })),
     ],
   }
@@ -105,7 +104,6 @@ function createUMDConfig(input, output) {
         },
       }),
       resolve({ extensions }),
-      typescript(),
       babelPlugin(getBabelOptions({ ie: 11 })),
     ],
   }
@@ -128,7 +126,6 @@ function createSystemConfig(input, output) {
         },
       }),
       resolve({ extensions }),
-      typescript(),
       getEsbuild('node12'),
     ],
   }
diff --git a/src/react.ts b/src/react.ts
index 2cc4762..34a9043 100644
--- a/src/react.ts
+++ b/src/react.ts
@@ -14,7 +14,13 @@ import {
 } from 'proxy-compare'
 import { createMutableSource, useMutableSource } from './useMutableSource'
 import { getVersion, snapshot, subscribe } from './vanilla'
-import type { DeepResolveType } from './vanilla'
+
+class SnapshotWrapper<T extends object> {
+  fn(p: T) {
+    return snapshot(p)
+  }
+}
+type Snapshot<T extends object> = ReturnType<SnapshotWrapper<T>['fn']>
 
 const isSSR =
   typeof window === 'undefined' ||
@@ -127,12 +133,12 @@ type Options = {
 export const useSnapshot = <T extends object>(
   proxyObject: T,
   options?: Options
-): DeepResolveType<T> => {
+): Snapshot<T> => {
   const forceUpdate = useReducer((c) => c + 1, 0)[1]
   const affected = new WeakMap()
   const lastAffected = useRef<typeof affected>()
-  const prevSnapshot = useRef<DeepResolveType<T>>()
-  const lastSnapshot = useRef<DeepResolveType<T>>()
+  const prevSnapshot = useRef<Snapshot<T>>()
+  const lastSnapshot = useRef<Snapshot<T>>()
   useIsomorphicLayoutEffect(() => {
     lastSnapshot.current = prevSnapshot.current = snapshot(proxyObject)
   }, [proxyObject])
diff --git a/src/utils/derive.ts b/src/utils/derive.ts
index b56df4c..d36a764 100644
--- a/src/utils/derive.ts
+++ b/src/utils/derive.ts
@@ -2,10 +2,12 @@ import { getVersion, proxy, subscribe } from '../vanilla'
 
 type DeriveGet = <T extends object>(proxyObject: T) => T
 
-type Subscriptions<U extends object> = Map<
-  object,
-  [callbackMap: Map<keyof U, () => void>, unsubscribe: () => void]
->
+type Subscription<U extends object> = [
+  callbackMap: Map<keyof U, () => void>,
+  unsubscribe: () => void
+]
+
+type Subscriptions<U extends object> = Map<object, Subscription<U>>
 
 const subscriptionsCache = new WeakMap<object, Subscriptions<object>>()
 
@@ -24,6 +26,25 @@ const getSubscriptions = (proxyObject: object) => {
 // It's not expected to use this in production.
 export const unstable_getDeriveSubscriptions = getSubscriptions
 
+// to make derive glitch free: https://github.com/pmndrs/valtio/pull/335
+const pendingCountMap = new WeakMap<object, number>()
+const markPending = (proxyObject: object) => {
+  const count = pendingCountMap.get(proxyObject) || 0
+  pendingCountMap.set(proxyObject, count + 1)
+}
+const unmarkPending = (proxyObject: object) => {
+  const count = pendingCountMap.get(proxyObject) || 0
+  if (count > 1) {
+    pendingCountMap.set(proxyObject, count - 1)
+  } else {
+    pendingCountMap.delete(proxyObject)
+  }
+}
+const isPending = (proxyObject: object) => {
+  const count = pendingCountMap.get(proxyObject) || 0
+  return count > 0
+}
+
 /**
  * derive
  *
@@ -61,10 +82,11 @@ export const derive = <T extends object, U extends object>(
   const notifyInSync = options?.sync
   const subscriptions: Subscriptions<U> = getSubscriptions(proxyObject)
   const addSubscription = (p: object, key: keyof U, callback: () => void) => {
-    const subscription = subscriptions.get(p)
-    if (subscription) {
-      subscription[0].set(key, callback)
-    } else {
+    let subscription = subscriptions.get(p)
+    if (!subscription) {
+      const notify = () =>
+        (subscription as Subscription<U>)[0].forEach((cb) => cb())
+      let promise: Promise<void> | undefined
       const unsubscribe = subscribe(
         p,
         (ops) => {
@@ -77,14 +99,28 @@ export const derive = <T extends object, U extends object>(
             // only setting derived properties
             return
           }
-          subscriptions.get(p)?.[0].forEach((cb) => {
-            cb()
-          })
+          if (promise) {
+            // already scheduled
+            return
+          }
+          markPending(p)
+          if (notifyInSync) {
+            unmarkPending(p)
+            notify()
+          } else {
+            promise = Promise.resolve().then(() => {
+              promise = undefined
+              unmarkPending(p)
+              notify()
+            })
+          }
         },
-        notifyInSync
+        true
       )
-      subscriptions.set(p, [new Map([[key, callback]]), unsubscribe])
+      subscription = [new Map(), unsubscribe]
+      subscriptions.set(p, subscription)
     }
+    subscription[0].set(key, callback)
   }
   const removeSubscription = (p: object, key: keyof U) => {
     const subscription = subscriptions.get(p)
@@ -105,6 +141,10 @@ export const derive = <T extends object, U extends object>(
     let lastDependencies: Map<object, number> | null = null
     const evaluate = () => {
       if (lastDependencies) {
+        if (Array.from(lastDependencies).some(([p]) => isPending(p))) {
+          // some dependencies are still pending
+          return
+        }
         if (
           Array.from(lastDependencies).every(([p, n]) => getVersion(p) === n)
         ) {
@@ -118,7 +158,7 @@ export const derive = <T extends object, U extends object>(
         return p
       }
       const value = fn(get)
-      const subscribe = () => {
+      const subscribeToDependencies = () => {
         dependencies.forEach((_, p) => {
           if (!lastDependencies?.has(p)) {
             addSubscription(p, key, evaluate)
@@ -132,9 +172,9 @@ export const derive = <T extends object, U extends object>(
         lastDependencies = dependencies
       }
       if (value instanceof Promise) {
-        value.finally(subscribe)
+        value.finally(subscribeToDependencies)
       } else {
-        subscribe()
+        subscribeToDependencies()
       }
       proxyObject[key] = value
     }
diff --git a/src/utils/proxyWithComputed.ts b/src/utils/proxyWithComputed.ts
index 9308f4e..8cfa39d 100644
--- a/src/utils/proxyWithComputed.ts
+++ b/src/utils/proxyWithComputed.ts
@@ -1,6 +1,12 @@
 import { createProxy as createProxyToCompare, isChanged } from 'proxy-compare'
 import { proxy, snapshot } from '../vanilla'
-import type { DeepResolveType } from '../vanilla'
+
+class SnapshotWrapper<T extends object> {
+  fn(p: T) {
+    return snapshot(p)
+  }
+}
+type Snapshot<T extends object> = ReturnType<SnapshotWrapper<T>['fn']>
 
 /**
  * proxyWithComputed
@@ -30,9 +36,9 @@ export const proxyWithComputed = <T extends object, U extends object>(
   initialObject: T,
   computedFns: {
     [K in keyof U]:
-      | ((snap: DeepResolveType<T>) => U[K])
+      | ((snap: Snapshot<T>) => U[K])
       | {
-          get: (snap: DeepResolveType<T>) => U[K]
+          get: (snap: Snapshot<T>) => U[K]
           set?: (state: T, newValue: U[K]) => void
         }
   }
@@ -45,11 +51,11 @@ export const proxyWithComputed = <T extends object, U extends object>(
     const { get, set } = (
       typeof computedFn === 'function' ? { get: computedFn } : computedFn
     ) as {
-      get: (snap: DeepResolveType<T>) => U[typeof key]
+      get: (snap: Snapshot<T>) => U[typeof key]
       set?: (state: T, newValue: U[typeof key]) => void
     }
     let computedValue: U[typeof key]
-    let prevSnapshot: DeepResolveType<T> | undefined
+    let prevSnapshot: Snapshot<T> | undefined
     let affected = new WeakMap()
     const desc: PropertyDescriptor = {}
     desc.get = () => {
diff --git a/src/vanilla.ts b/src/vanilla.ts
index 05e8954..e21e9e0 100644
--- a/src/vanilla.ts
+++ b/src/vanilla.ts
@@ -7,7 +7,7 @@ const HANDLER = Symbol()
 const PROMISE_RESULT = Symbol()
 const PROMISE_ERROR = Symbol()
 
-const enum AsRef {}
+type AsRef = { $$valtioRef: true }
 const refSet = new WeakSet()
 export const ref = <T extends object>(o: T): T & AsRef => {
   refSet.add(o)
@@ -247,6 +247,9 @@ export const subscribe = <T extends object>(
   }
 }
 
+/**
+ * @deprecated this will be removed in next versions
+ */
 export type DeepResolveType<T> = T extends (...args: any[]) => any
   ? T
   : T extends AsRef
@@ -259,9 +262,18 @@ export type DeepResolveType<T> = T extends (...args: any[]) => any
     }
   : T
 
-export const snapshot = <T extends object>(
-  proxyObject: T
-): DeepResolveType<T> => {
+type AnyFunction = (...args: any[]) => any
+type Snapshot<T> = T extends AnyFunction
+  ? T
+  : T extends AsRef
+  ? T
+  : T extends Promise<infer V>
+  ? Snapshot<V>
+  : {
+      readonly [K in keyof T]: Snapshot<T[K]>
+    }
+
+export const snapshot = <T extends object>(proxyObject: T): Snapshot<T> => {
   if (
     typeof process === 'object' &&
     process.env.NODE_ENV !== 'production' &&
diff --git a/yarn.lock b/yarn.lock
index b1d0987..f01f686 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -528,7 +528,7 @@
   dependencies:
     "@babel/helper-plugin-utils" "^7.14.5"
 
-"@babel/plugin-syntax-typescript@^7.7.2":
+"@babel/plugin-syntax-typescript@^7.16.7", "@babel/plugin-syntax-typescript@^7.7.2":
   version "7.16.7"
   resolved "https://registry.yarnpkg.com/@babel/plugin-syntax-typescript/-/plugin-syntax-typescript-7.16.7.tgz#39c9b55ee153151990fb038651d58d3fd03f98f8"
   integrity sha512-YhUIJHHGkqPgEcMYkPCKTyGUdoGKWtopIycQyjJH8OjvRgOYsXsaKehLVPScKJWAULPxMa4N1vCe6szREFlZ7A==
@@ -781,6 +781,15 @@
   dependencies:
     "@babel/helper-plugin-utils" "^7.16.7"
 
+"@babel/plugin-transform-typescript@^7.16.8":
+  version "7.16.8"
+  resolved "https://registry.yarnpkg.com/@babel/plugin-transform-typescript/-/plugin-transform-typescript-7.16.8.tgz#591ce9b6b83504903fa9dd3652c357c2ba7a1ee0"
+  integrity sha512-bHdQ9k7YpBDO2d0NVfkj51DpQcvwIzIusJ7mEUaMlbZq3Kt/U47j24inXZHQ5MDiYpCs+oZiwnXyKedE8+q7AQ==
+  dependencies:
+    "@babel/helper-create-class-features-plugin" "^7.16.7"
+    "@babel/helper-plugin-utils" "^7.16.7"
+    "@babel/plugin-syntax-typescript" "^7.16.7"
+
 "@babel/plugin-transform-unicode-escapes@^7.16.7":
   version "7.16.7"
   resolved "https://registry.yarnpkg.com/@babel/plugin-transform-unicode-escapes/-/plugin-transform-unicode-escapes-7.16.7.tgz#da8717de7b3287a2c6d659750c964f302b31ece3"


Test patch:
diff --git a/tests/derive.test.tsx b/tests/derive.test.tsx
index 79557aa..21570f1 100644
--- a/tests/derive.test.tsx
+++ b/tests/derive.test.tsx
@@ -211,7 +211,7 @@ it('nested emulation with derive', async () => {
     {
       doubled: (get) => computeDouble(get(state.math).count),
     },
-    { proxy: state.math }
+    { proxy: state.math, sync: true }
   )
 
   const callback = jest.fn()
@@ -299,3 +299,135 @@ it('basic underive', async () => {
   await Promise.resolve()
   expect(callback).toBeCalledTimes(1)
 })
+
+describe('glitch free', () => {
+  it('basic (#296)', async () => {
+    const state = proxy({ value: 0 })
+    const derived1 = derive({
+      value: (get) => get(state).value,
+    })
+    const derived2 = derive({
+      value: (get) => get(derived1).value,
+    })
+    const computeValue = jest.fn((get) => {
+      const v0 = get(state).value
+      const v1 = get(derived1).value
+      const v2 = get(derived2).value
+      return v0 + (v1 - v2)
+    })
+    const derived3 = derive({
+      value: (get) => computeValue(get),
+    })
+
+    const App = () => {
+      const snap = useSnapshot(derived3)
+      return (
+        <div>
+          value: {snap.value}
+          <button onClick={() => ++state.value}>button</button>
+        </div>
+      )
+    }
+
+    const { getByText, findByText } = render(
+      <StrictMode>
+        <App />
+      </StrictMode>
+    )
+
+    await findByText('value: 0')
+    expect(computeValue).toBeCalledTimes(1)
+
+    fireEvent.click(getByText('button'))
+    await findByText('value: 1')
+    expect(computeValue).toBeCalledTimes(2)
+  })
+
+  it('same value', async () => {
+    const state = proxy({ value: 0 })
+    const derived1 = derive({
+      value: (get) => get(state).value * 0,
+    })
+    const derived2 = derive({
+      value: (get) => get(derived1).value * 0,
+    })
+    const computeValue = jest.fn((get) => {
+      const v0 = get(state).value
+      const v1 = get(derived1).value
+      const v2 = get(derived2).value
+      return v0 + (v1 - v2)
+    })
+    const derived3 = derive({
+      value: (get) => computeValue(get),
+    })
+
+    const App = () => {
+      const snap = useSnapshot(derived3)
+      return (
+        <div>
+          value: {snap.value}
+          <button onClick={() => ++state.value}>button</button>
+        </div>
+      )
+    }
+
+    const { getByText, findByText } = render(
+      <StrictMode>
+        <App />
+      </StrictMode>
+    )
+
+    await findByText('value: 0')
+    expect(computeValue).toBeCalledTimes(1)
+
+    fireEvent.click(getByText('button'))
+    await findByText('value: 1')
+    expect(computeValue).toBeCalledTimes(2)
+  })
+
+  it('double chain', async () => {
+    const state = proxy({ value: 0 })
+    const derived1 = derive({
+      value: (get) => get(state).value,
+    })
+    const derived2 = derive({
+      value: (get) => get(derived1).value,
+    })
+    const derived3 = derive({
+      value: (get) => get(derived2).value,
+    })
+    const computeValue = jest.fn((get) => {
+      const v0 = get(state).value
+      const v1 = get(derived1).value
+      const v2 = get(derived2).value
+      const v3 = get(derived3).value
+      return v0 + (v1 - v2) + v3 * 0
+    })
+    const derived4 = derive({
+      value: (get) => computeValue(get),
+    })
+
+    const App = () => {
+      const snap = useSnapshot(derived4)
+      return (
+        <div>
+          value: {snap.value}
+          <button onClick={() => ++state.value}>button</button>
+        </div>
+      )
+    }
+
+    const { getByText, findByText } = render(
+      <StrictMode>
+        <App />
+      </StrictMode>
+    )
+
+    await findByText('value: 0')
+    expect(computeValue).toBeCalledTimes(1)
+
+    fireEvent.click(getByText('button'))
+    await findByText('value: 1')
+    expect(computeValue).toBeCalledTimes(2)
+  })
+})