feat(refs): do not rebind already bound refs

Some refactoring slipped in as well. Sinc subscribeToRefs can be called on an object with refs that
has already been bound, we must check when we bind that it hasn't been done yet. Because subs are
saved with refKey (obj.nested.imaref), we should be safe to skip binding

Author: posva

src/index.js

@@ -1,5 +1,60 @@
 import { createSnapshot, extractRefs, callOnceWithArg, deepGetSplit } from './utils'
 
+// NOTE not convinced by the naming of subscribeToRefs and subscribeToDocument
+// first one is calling the other on every ref and subscribeToDocument may call
+// updateDataFromDocumentSnapshot which may call subscribeToRefs as well
+function subscribeToRefs ({
+  subs,
+  refs,
+  target,
+  key,
+  data,
+  depth,
+  resolve
+}) {
+  const refKeys = Object.keys(refs)
+  if (!refKeys.length) return resolve()
+  // TODO check if no ref is missing
+  // TODO max depth param, default to 1?
+  if (++depth > 3) throw new Error('more than 5 nested refs')
+  refKeys.forEach(refKey => {
+    // check if already bound to the same ref -> skip
+    // TODO reuse if already bound?
+    const sub = subs[refKey]
+    const ref = refs[refKey]
+
+    if (sub) {
+      if (sub.path !== ref.path) {
+        sub.unbind()
+      } else {
+        // skip it as it's already bound
+        // NOTE this is valid as long as target is the same
+        // which is not checked anywhere but should be ok
+        // because the subs object is created when needed
+        return
+      }
+    }
+
+    // maybe wrap the unbind function to call unbind on every child
+    const [innerObj, innerKey] = deepGetSplit(target[key], refKey)
+    if (!innerObj) {
+      console.log('=== ERROR ===')
+      console.log(data, refKey, key, innerObj, innerKey)
+      console.log('===')
+    }
+    subs[refKey] = {
+      unbind: subscribeToDocument({
+        ref,
+        target: innerObj,
+        key: innerKey,
+        depth,
+        resolve
+      }),
+      path: ref.path
+    }
+  })
+}
+
 function bindCollection ({
   vm,
   key,
@@ -9,48 +64,21 @@ function bindCollection ({
 }) {
   // TODO wait to get all data
   const array = vm[key] = []
-  const depth = 0
 
   const change = {
     added: ({ newIndex, doc }) => {
       const subs = {}
       const snapshot = createSnapshot(doc)
       const [data, refs] = extractRefs(snapshot)
       array.splice(newIndex, 0, data)
-      const refKeys = Object.keys(refs)
-      if (!refKeys.length) return // resolve()
-      // TODO check if no ref is missing
-      // TODO max depth param, default to 1?
-      // if (++depth > 3) throw new Error('more than 5 nested refs')
-      refKeys.forEach(refKey => {
-        // check if already bound to the same ref -> skip
-        const sub = subs[refKey]
-        const ref = refs[refKey]
-        if (sub && sub.path !== ref.path) {
-          sub.unbind()
-        }
-        // maybe wrap the unbind function to call unbind on every child
-        const [innerObj, innerKey] = deepGetSplit(array[newIndex], refKey)
-        if (!innerObj) {
-          console.log('=== ERROR ===')
-          console.log(data, refKey, newIndex, innerObj, innerKey)
-          console.log('===')
-        }
-        subs[refKey] = {
-          unbind: subscribeToDocument({
-            ref,
-            target: innerObj,
-            key: innerKey,
-            depth,
-            // TODO parentSubs
-            resolve
-          }),
-          path: ref.path
-        }
-        // unbind currently bound ref
-        // bind ref
-        // save unbind callback
-        // probably save key or something as well
+      subscribeToRefs({
+        data,
+        refs,
+        subs,
+        target: array,
+        key: newIndex,
+        depth: 0,
+        resolve
       })
     },
     modified: ({ oldIndex, newIndex, doc }) => {
@@ -80,38 +108,16 @@ function bindCollection ({
 }
 
 function updateDataFromDocumentSnapshot ({ snapshot, target, key, subs, depth = 0, resolve }) {
-  // TODO extract refs
   const [data, refs] = extractRefs(snapshot)
   target[key] = data
-  const refKeys = Object.keys(refs)
-  if (!refKeys.length) return resolve()
-  // TODO check if no ref is missing
-  // TODO max depth param, default to 1?
-  if (++depth > 3) throw new Error('more than 5 nested refs')
-  refKeys.forEach(refKey => {
-    // check if already bound to the same ref -> skip
-    const sub = subs[refKey]
-    const ref = refs[refKey]
-    if (sub && sub.path !== ref.path) {
-      sub.unbind()
-    }
-    // maybe wrap the unbind function to call unbind on every child
-    const [innerObj, innerKey] = deepGetSplit(target[key], refKey)
-    subs[refKey] = {
-      unbind: subscribeToDocument({
-        ref,
-        target: innerObj,
-        key: innerKey,
-        depth,
-        // TODO parentSubs
-        resolve
-      }),
-      path: ref.path
-    }
-    // unbind currently bound ref
-    // bind ref
-    // save unbind callback
-    // probably save key or something as well
+  subscribeToRefs({
+    data,
+    subs,
+    refs,
+    target,
+    key,
+    depth,
+    resolve
   })
 }
 

test/refs-documents.spec.js

@@ -40,6 +40,34 @@ beforeEach(async () => {
   await delay(5)
 })
 
+function spyUnbind (ref) {
+  const spy = jest.fn()
+  const onSnapshot = ref.onSnapshot.bind(ref)
+  ref.onSnapshot = fn => {
+    const unbind = onSnapshot(fn)
+    return () => {
+      spy()
+      unbind()
+    }
+  }
+  return spy
+}
+
+function spyOnSnapshot (ref) {
+  const onSnapshot = ref.onSnapshot.bind(ref)
+  return (ref.onSnapshot = jest.fn((...args) => onSnapshot(...args)))
+}
+
+function spyOnSnapshotCallback (ref) {
+  const onSnapshot = ref.onSnapshot.bind(ref)
+  const spy = jest.fn()
+  ref.onSnapshot = fn => onSnapshot((...args) => {
+    spy()
+    fn(...args)
+  })
+  return spy
+}
+
 test('binds refs on documents', async () => {
   // create an empty doc and update using the ref instead of plain data
   const c = collection.doc()
@@ -129,12 +157,7 @@ test('unbinds previously bound document when overwriting a bound', async () => {
   const c = collection.doc()
 
   // Mock c onSnapshot to spy when the callback is called
-  const spy = jest.fn()
-  const onSnapshot = c.onSnapshot.bind(c)
-  c.onSnapshot = jest.fn(fn => onSnapshot((...args) => {
-    spy()
-    fn(...args)
-  }))
+  const spy = spyOnSnapshotCallback(c)
   await c.update({ baz: 'baz' })
   await d.update({ ref: c })
   await delay(5)
@@ -163,6 +186,22 @@ test('unbinds previously bound document when overwriting a bound', async () => {
   spy.mockRestore()
 })
 
+test('does not rebind if it is the same ref', async () => {
+  const c = collection.doc()
+
+  const spy = spyOnSnapshot(c)
+  await c.update({ baz: 'baz' })
+  await d.update({ ref: c })
+  await delay(5)
+  expect(spy).toHaveBeenCalledTimes(1)
+
+  await d.update({ ref: c })
+  await delay(5)
+
+  expect(spy).toHaveBeenCalledTimes(1)
+  spy.mockRestore()
+})
+
 test('resolves the promise when refs are resolved in a document', async () => {
   await a.update({ a: true })
   await b.update({ ref: a })
@@ -194,19 +233,6 @@ test('resolves the promise when the document does not exist', async () => {
   expect(vm.item).toBe(null)
 })
 
-function spyUnbind (ref) {
-  const spy = jest.fn()
-  const onSnapshot = ref.onSnapshot.bind(ref)
-  ref.onSnapshot = jest.fn(fn => {
-    const unbind = onSnapshot(fn)
-    return () => {
-      spy()
-      unbind()
-    }
-  })
-  return spy
-}
-
 test('unbinds all refs when the document is unbound', async () => {
   const cSpy = spyUnbind(c)
   const dSpy = spyUnbind(d)