Repository: pmndrs/valtio
Pull request #56 (2020-12-24 09:46:20)

fix: better render invocation with scoped subscription
While I was working on #54, I noticed a slight unexpected behavior. A hook can call render function several times when another hook bails out.
Turns out we should use scope subscription instead. This PR fixes it and adds the test spec for the expected behavior.

codesandbox[bot]: This pull request is automatically built and testable in [CodeSandbox](https://codesandbox.io).

  To see build info of the built libraries, click [here](https://ci.codesandbox.io/status/pmndrs/valtio/pr/56/builds/85669) or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7c33c0fa0f937b11d8f61bb0da4fa985abcc3d70:

|Sandbox| Source |
|--|--|
|[React](https://codesandbox.io/s/react-25gu3)| Configuration |
|[React Typescript](https://codesandbox.io/s/react-typescript-r8n91)| Configuration |

dai-shi: https://codesandbox.io/s/damp-river-8tkvu
Checked the example in #46 still works.

Patch:
diff --git a/.size-snapshot.json b/.size-snapshot.json
index 3e5fae9..569b56c 100644
--- a/.size-snapshot.json
+++ b/.size-snapshot.json
@@ -1,8 +1,8 @@
 {
   "index.js": {
-    "bundled": 9945,
-    "minified": 4620,
-    "gzipped": 1704,
+    "bundled": 9994,
+    "minified": 4624,
+    "gzipped": 1698,
     "treeshaked": {
       "rollup": {
         "code": 137,
@@ -14,14 +14,14 @@
     }
   },
   "index.cjs.js": {
-    "bundled": 10771,
-    "minified": 5080,
-    "gzipped": 1797
+    "bundled": 10855,
+    "minified": 5089,
+    "gzipped": 1800
   },
   "index.iife.js": {
-    "bundled": 11442,
-    "minified": 4045,
-    "gzipped": 1606
+    "bundled": 11534,
+    "minified": 4044,
+    "gzipped": 1602
   },
   "vanilla.js": {
     "bundled": 5470,
diff --git a/src/index.ts b/src/index.ts
index 3c6a988..e5f4b80 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -1,5 +1,4 @@
 import {
-  useCallback,
   useDebugValue,
   useEffect,
   useLayoutEffect,
@@ -52,6 +51,10 @@ const useProxy = <T extends object>(p: T, options?: Options): NonPromise<T> => {
   const lastAffected = useRef<typeof affected>()
   const prevSnapshot = useRef<NonPromise<T>>()
   const lastSnapshot = useRef<NonPromise<T>>()
+  if (!lastSnapshot.current) {
+    // lazy initialization
+    lastSnapshot.current = prevSnapshot.current = snapshot(p)
+  }
   useIsomorphicLayoutEffect(() => {
     lastAffected.current = affected
     if (
@@ -67,37 +70,38 @@ const useProxy = <T extends object>(p: T, options?: Options): NonPromise<T> => {
       forceUpdate()
     }
   })
-  const getSnapshot = useMemo(() => {
-    const deepChangedCache = new WeakMap()
-    return (p: T) => {
-      const nextSnapshot = snapshot(p)
-      lastSnapshot.current = nextSnapshot
-      try {
-        if (
-          prevSnapshot &&
-          lastAffected.current &&
-          !isDeepChanged(
-            prevSnapshot.current,
-            nextSnapshot,
-            lastAffected.current,
-            deepChangedCache
-          )
-        ) {
-          // not changed
-          return prevSnapshot.current
-        }
-      } catch (e) {
-        // ignore and return nextSnapshot
-      }
-      return (prevSnapshot.current = nextSnapshot)
-    }
-  }, [])
   const notifyInSync = options?.sync
-  const sub = useCallback(
-    (p: T, cb: () => void) => subscribe(p, cb, notifyInSync),
-    [notifyInSync]
-  )
-  const currSnapshot = useMutableSource(getMutableSource(p), getSnapshot, sub)
+  const sub = useMemo(() => {
+    const deepChangedCache = new WeakMap()
+    return (p: T, cb: () => void) =>
+      subscribe(
+        p,
+        () => {
+          const nextSnapshot = snapshot(p)
+          lastSnapshot.current = nextSnapshot
+          try {
+            if (
+              lastAffected.current &&
+              !isDeepChanged(
+                prevSnapshot.current,
+                nextSnapshot,
+                lastAffected.current,
+                deepChangedCache
+              )
+            ) {
+              // not changed
+              return
+            }
+          } catch (e) {
+            // ignore if a promise or something is thrown
+          }
+          prevSnapshot.current = nextSnapshot
+          cb()
+        },
+        notifyInSync
+      )
+  }, [notifyInSync])
+  const currSnapshot = useMutableSource(getMutableSource(p), snapshot, sub)
   if (typeof process === 'object' && process.env.NODE_ENV !== 'production') {
     // eslint-disable-next-line react-hooks/rules-of-hooks
     useAffectedDebugValue(currSnapshot, affected)


Test patch:
diff --git a/tests/basic.test.tsx b/tests/basic.test.tsx
index ec3fffe..0918bc0 100644
--- a/tests/basic.test.tsx
+++ b/tests/basic.test.tsx
@@ -87,6 +87,90 @@ it('no extra re-renders (commits)', async () => {
   })
 })
 
+it('no extra re-renders (render func calls in non strict mode)', async () => {
+  const obj = proxy({ count: 0, count2: 0 })
+
+  const renderFn = jest.fn()
+  const Counter: React.FC = () => {
+    const snapshot = useProxy(obj)
+    renderFn(snapshot.count)
+    return (
+      <>
+        <div>count: {snapshot.count}</div>
+        <button onClick={() => ++obj.count}>button</button>
+      </>
+    )
+  }
+
+  const renderFn2 = jest.fn()
+  const Counter2: React.FC = () => {
+    const snapshot = useProxy(obj)
+    renderFn2(snapshot.count2)
+    return (
+      <>
+        <div>count2: {snapshot.count2}</div>
+        <button onClick={() => ++obj.count2}>button2</button>
+      </>
+    )
+  }
+
+  const { getByText } = render(
+    <>
+      <Counter />
+      <Counter2 />
+    </>
+  )
+
+  await waitFor(() => {
+    getByText('count: 0')
+    getByText('count2: 0')
+  })
+  expect(renderFn).toBeCalledTimes(1)
+  expect(renderFn).lastCalledWith(0)
+  expect(renderFn2).toBeCalledTimes(1)
+  expect(renderFn2).lastCalledWith(0)
+
+  fireEvent.click(getByText('button'))
+  await waitFor(() => {
+    getByText('count: 1')
+    getByText('count2: 0')
+  })
+  expect(renderFn).toBeCalledTimes(2)
+  expect(renderFn).lastCalledWith(1)
+  expect(renderFn2).toBeCalledTimes(1)
+  expect(renderFn2).lastCalledWith(0)
+
+  fireEvent.click(getByText('button2'))
+  await waitFor(() => {
+    getByText('count: 1')
+    getByText('count2: 1')
+  })
+  expect(renderFn).toBeCalledTimes(2)
+  expect(renderFn).lastCalledWith(1)
+  expect(renderFn2).toBeCalledTimes(2)
+  expect(renderFn2).lastCalledWith(1)
+
+  fireEvent.click(getByText('button2'))
+  await waitFor(() => {
+    getByText('count: 1')
+    getByText('count2: 2')
+  })
+  expect(renderFn).toBeCalledTimes(2)
+  expect(renderFn).lastCalledWith(1)
+  expect(renderFn2).toBeCalledTimes(3)
+  expect(renderFn2).lastCalledWith(2)
+
+  fireEvent.click(getByText('button'))
+  await waitFor(() => {
+    getByText('count: 2')
+    getByText('count2: 2')
+  })
+  expect(renderFn).toBeCalledTimes(3)
+  expect(renderFn).lastCalledWith(2)
+  expect(renderFn2).toBeCalledTimes(3)
+  expect(renderFn2).lastCalledWith(2)
+})
+
 it('object in object', async () => {
   const obj = proxy({ object: { count: 0 } })