refactor: reivew usesubscription with usedeepcompareeffect

packages/graphql-hooks/src/useDeepCompareEffect.ts

@@ -0,0 +1,27 @@
+import * as React from 'react'
+import { dequal } from 'dequal'
+
+type UseEffectParams = Parameters<typeof React.useEffect>
+type EffectCallback = UseEffectParams[0]
+type DependencyList = NonNullable<UseEffectParams[1]>
+type UseEffectReturn = ReturnType<typeof React.useEffect>
+
+export function useDeepCompareMemoize<T>(value: T) {
+  const ref = React.useRef<T>(value)
+  const signalRef = React.useRef<number>(0)
+
+  if (!dequal(value, ref.current)) {
+    ref.current = value
+    signalRef.current += 1
+  }
+  return React.useMemo(() => ref.current, [signalRef.current])
+}
+
+function useDeepCompareEffect(
+  callback: EffectCallback,
+  dependencies: DependencyList = []
+): UseEffectReturn {
+  return React.useEffect(callback, useDeepCompareMemoize(dependencies))
+}
+
+export default useDeepCompareEffect

packages/graphql-hooks/src/useSubscription.ts

@@ -1,7 +1,7 @@
 import { useContext, useRef, useEffect } from 'react'
 
 import ClientContext from './ClientContext'
-import { isEqualFirstLevel } from './utils'
+import useDeepCompareEffect from './useDeepCompareEffect'
 
 import { UseSubscriptionOperation } from './types/common-types'
 
@@ -28,22 +28,10 @@ function useSubscription<
     )
   }
 
-  // we need a persistent reference to the variables object to compare against new versions, and to use as a `useEffect` dependency
-  const variablesRef = useRef<Variables | undefined>()
-
-  // we check the new variables object outside of any hook to prevent useEffect from trying to call the unsubscribe return function when it shouldn't
-  if (
-    !variablesRef.current ||
-    !options.variables ||
-    !isEqualFirstLevel(variablesRef.current, options.variables)
-  ) {
-    variablesRef.current = options.variables
-  }
-
-  useEffect(() => {
+  useDeepCompareEffect(() => {
     const request = {
       query: options.query,
-      variables: variablesRef.current
+      variables: options.variables
     }
 
     const observable = client.createSubscription(request)
@@ -63,7 +51,7 @@ function useSubscription<
     return () => {
       subscription.unsubscribe()
     }
-  }, [options.query, variablesRef.current])
+  }, [options.query, options.variables])
 }
 
 export default useSubscription

packages/graphql-hooks/src/utils.ts

@@ -5,35 +5,3 @@
  */
 export const pipeP = (fns: (() => any)[]) => (arg: any) =>
   fns.reduce((p, f) => p.then(f), Promise.resolve(arg))
-
-/**
- * Compares two objects for equality _only considering the first level of object keys_.
- * @example
- * var o1 = {a: 1, b: 2}
- * var o2 = {a: 1, b: 2}
- * isEqual(o1, o2) === true
- *
- * var o1 = {a: 2, b: 4}
- * var o2 = {a: 9, b: 'fish', c: 'why is this here?'}
- * isEqual(o1, o2) === false
- */
-export function isEqualFirstLevel<T extends Record<string, any>>(
-  obj1: T,
-  obj2: T
-): boolean {
-  if (obj1 === obj2) return true
-
-  const keys1 = Object.keys(obj1)
-  const keys2 = Object.keys(obj2)
-
-  if (keys1.length !== keys2.length) {
-    return false
-  }
-
-  for (let i = 0; i < keys1.length; i++) {
-    const key = keys2[i]
-    if (!(key in obj2) || !Object.is(obj1[key], obj2[key])) return false
-  }
-
-  return true
-}

packages/graphql-hooks/test-jsdom/unit/useDeepCompareEffect.test.ts

@@ -0,0 +1,123 @@
+import { renderHook } from '@testing-library/react'
+import useDeepCompareEffect from '../../src/useDeepCompareEffect'
+
+describe('useDeepCompareEffect', () => {
+  let callback: jest.Mock
+
+  beforeEach(() => {
+    callback = jest.fn()
+  })
+
+  describe('onMount', () => {
+    it('should call the callback once on mount if deps are empty', () => {
+      renderHook(({ deps }) => useDeepCompareEffect(callback, deps), {
+        initialProps: { deps: [] }
+      })
+      expect(callback).toHaveBeenCalledTimes(1)
+    })
+
+    it('should call the callback once on mount if deps are not empty', () => {
+      renderHook(({ deps }) => useDeepCompareEffect(callback, deps), {
+        initialProps: { deps: [{ a: 1 }, { b: 1 }] }
+      })
+      expect(callback).toHaveBeenCalledTimes(1)
+    })
+  })
+
+  it('should call the callback only once on mount if deps are undefined', () => {
+    const { rerender } = renderHook(
+      ({ deps }) => useDeepCompareEffect(callback, deps),
+      {
+        initialProps: { deps: undefined as unknown as unknown[] }
+      }
+    )
+    expect(callback).toHaveBeenCalledTimes(1)
+
+    rerender({ deps: undefined as unknown as unknown[] })
+    expect(callback).toHaveBeenCalledTimes(1)
+  })
+
+  it('should call the callback once if deps change but have same values', () => {
+    const { rerender } = renderHook(
+      ({ deps }) => useDeepCompareEffect(callback, deps),
+      {
+        initialProps: { deps: [{ a: 1, b: 3 }, { c: 2 }] }
+      }
+    )
+
+    rerender({ deps: [{ a: 1, b: 3 }, { c: 2 }] })
+    expect(callback).toHaveBeenCalledTimes(1)
+  })
+
+  it('should call the callback twice when a dependency changes', () => {
+    const { rerender } = renderHook(
+      ({ deps }) => useDeepCompareEffect(callback, deps),
+      {
+        initialProps: { deps: [{ a: 1 }] }
+      }
+    )
+
+    rerender({ deps: [{ a: 2 }] })
+    expect(callback).toHaveBeenCalledTimes(2)
+  })
+
+  it('should call the callback once when a dependency contains a list of undefined', () => {
+    const { rerender } = renderHook(
+      ({ deps }) => useDeepCompareEffect(callback, deps),
+      {
+        initialProps: { deps: [undefined, undefined] }
+      }
+    )
+
+    rerender({ deps: [undefined, undefined] })
+    expect(callback).toHaveBeenCalledTimes(1)
+  })
+
+  it('should call the callback twice when dependency changes from an undefined value to a defined value', () => {
+    const { rerender } = renderHook(
+      ({ deps }) => useDeepCompareEffect(callback, deps),
+      {
+        initialProps: { deps: [undefined] as unknown[] }
+      }
+    )
+
+    rerender({ deps: [{ a: 1 }] })
+    expect(callback).toHaveBeenCalledTimes(2)
+  })
+
+  it('should call the callback twice when dependency changes from a defined value to an undefined value', () => {
+    const { rerender } = renderHook(
+      ({ deps }) => useDeepCompareEffect(callback, deps),
+      {
+        initialProps: { deps: [{ a: 1 }] as unknown[] }
+      }
+    )
+
+    rerender({ deps: [undefined] })
+    expect(callback).toHaveBeenCalledTimes(2)
+  })
+
+  it('should call the callback twice when dependency with many levels changes', () => {
+    const { rerender } = renderHook(
+      ({ deps }) => useDeepCompareEffect(callback, deps),
+      {
+        initialProps: { deps: [{ a: { b: { c: 3, d: 4 } } }] as unknown[] }
+      }
+    )
+
+    rerender({ deps: [{ a: { b: { c: 3 } } }] })
+    expect(callback).toHaveBeenCalledTimes(2)
+  })
+
+  it('should call the callback once when dependency with many levels is cloned', () => {
+    const { rerender } = renderHook(
+      ({ deps }) => useDeepCompareEffect(callback, deps),
+      {
+        initialProps: { deps: [{ a: { b: { c: 3 } } }] as unknown[] }
+      }
+    )
+
+    rerender({ deps: [{ a: { b: { c: 3 } } }] })
+    expect(callback).toHaveBeenCalledTimes(1)
+  })
+})

packages/graphql-hooks/test-jsdom/unit/utils.test.ts

@@ -44,31 +44,4 @@ describe('GraphQLClient utils', () => {
       expect(mock.mock.calls).toEqual([['fnA'], ['fnB'], ['fnC']])
     })
   })
-
-  describe('isEqualFirstLevel', () => {
-    it('returns true for two empty objects', () => {
-      expect(isEqualFirstLevel({}, {})).toBe(true)
-    })
-
-    it('returns true for two objects with the same keys and values', () => {
-      expect(isEqualFirstLevel({ a: 1, b: 2 }, { a: 1, b: 2 })).toBe(true)
-    })
-
-    it('returns true for two objects with the same keys and values but different keys order', () => {
-      expect(isEqualFirstLevel({ a: 1, b: 2 }, { b: 2, a: 1 })).toBe(true)
-    })
-
-    it('returns true with the same object', () => {
-      const obj = { a: 1, b: 2 }
-      expect(isEqualFirstLevel(obj, obj)).toBe(true)
-    })
-
-    it('returns false for two objects with different keys', () => {
-      expect(isEqualFirstLevel({ a: 1, b: 2 }, { a: 1, c: 2 })).toBe(false)
-    })
-
-    it('returns false for two objects with different values', () => {
-      expect(isEqualFirstLevel({ a: 1, b: 2 }, { a: 1, b: 3 })).toBe(false)
-    })
-  })
 })