fix(firestore): apply converter to nested refs

Fix #1263

Author: posva

src/firestore/bind.ts

@@ -55,7 +55,7 @@ export interface FirestoreRefOptions extends _DataSourceOptions {
  * Type of the global options for firestore refs. Some values cannot be `undefined`.
  * @internal
  */
-interface _FirestoreRefOptionsWithDefaults extends FirestoreRefOptions {
+export interface _FirestoreRefOptionsWithDefaults extends FirestoreRefOptions {
   /**
    * @defaultValue `false`
    */
@@ -121,11 +121,12 @@ function updateDataFromDocumentSnapshot<T>(
   reject: _ResolveRejectFn
 ) {
   const [data, refs] = extractRefs(
-    // @ts-expect-error: FIXME: use better types
     // Pass snapshot options
+    // @ts-expect-error: FIXME: use better types
     snapshot.data(options.snapshotOptions),
     walkGet(target, path),
-    subs
+    subs,
+    options
   )
   ops.set(target, path, data)
   subscribeToRefs(
@@ -319,7 +320,8 @@ export function bindCollection<T = unknown>(
         // @ts-expect-error: FIXME: wrong cast, needs better types
         doc.data(snapshotOptions),
         undefined,
-        subs
+        subs,
+        options
       )
       ops.add(unref(arrayRef), newIndex, data)
       subscribeToRefs(
@@ -342,7 +344,8 @@ export function bindCollection<T = unknown>(
         // @ts-expect-error: FIXME: Better types
         doc.data(snapshotOptions),
         oldData,
-        subs
+        subs,
+        options
       )
       // only move things around after extracting refs
       // only move things around after extracting refs

src/firestore/useFirestoreRef.ts

@@ -4,6 +4,7 @@ import type {
   CollectionReference,
   FirestoreError,
   DocumentData,
+  FirestoreDataConverter,
 } from 'firebase/firestore'
 import {
   unref,
@@ -38,13 +39,14 @@ import {
   bindDocument,
   firestoreOptionsDefaults,
   FirestoreRefOptions,
+  _FirestoreRefOptionsWithDefaults,
 } from './bind'
 
 export interface _UseFirestoreRefOptions extends FirestoreRefOptions {
   /**
    * @deprecated: use `.withConverter()` instead
    */
-  converter?: any
+  converter?: FirestoreDataConverter<unknown>
 }
 
 /**

src/firestore/utils.ts

@@ -9,6 +9,8 @@ import {
 } from 'firebase/firestore'
 import { isObject, isDocumentRef, TODO, isPOJO } from '../shared'
 import { VueFirestoreDocumentData } from '.'
+import type { _UseFirestoreRefOptions } from './useFirestoreRef'
+import { _FirestoreRefOptionsWithDefaults } from './bind'
 
 export type FirestoreReference = Query | DocumentReference | CollectionReference
 
@@ -40,7 +42,8 @@ export function extractRefs(
   // TODO: should be unknown instead of DocumentData
   doc: DocumentData,
   oldDoc: DocumentData | void,
-  subs: Record<string, { path: string; data: () => DocumentData | null }>
+  subs: Record<string, { path: string; data: () => DocumentData | null }>,
+  options: _FirestoreRefOptionsWithDefaults
 ): [DocumentData, Record<string, DocumentReference>] {
   if (!isPOJO(doc)) return [doc, {}]
 
@@ -98,7 +101,11 @@ export function extractRefs(
           // https://github.com/vuejs/vuefire/pull/1223
           refSubKey in subs ? oldDoc[key] : ref.path
         // TODO: handle subpathes?
-        refs[refSubKey] = ref
+        refs[refSubKey] = ref.converter
+          ? ref
+          : ref.withConverter(
+              options.converter as FirestoreDataConverter<DocumentData>
+            )
       } else if (Array.isArray(ref)) {
         data[key] = Array(ref.length)
         // fill existing refs into data but leave the rest empty

tests/firestore/refs-in-documents.spec.ts

@@ -7,7 +7,7 @@ import {
 } from 'firebase/firestore'
 import { setupFirestoreRefs, sleep } from '../utils'
 import { unref } from 'vue'
-import { _RefFirestore } from '../../src/firestore'
+import { VueFirestoreDocumentData, _RefFirestore } from '../../src/firestore'
 import {
   UseDocumentOptions,
   VueFirestoreQueryData,
@@ -26,7 +26,7 @@ describe('Firestore refs in documents', async () => {
     options?: UseDocumentOptions
     ref?: _MaybeRef<DocumentReference<T>>
   } = {}) {
-    let data!: _RefFirestore<VueFirestoreQueryData<T>>
+    let data!: _RefFirestore<VueFirestoreDocumentData<T>>
 
     const wrapper = mount({
       template: 'no',
@@ -116,6 +116,18 @@ describe('Firestore refs in documents', async () => {
     })
   })
 
+  it('refs are also serialized with the converter', async () => {
+    const docRef = await addDoc(listOfRefs, { a: aRef })
+    const { data, pending, promise } = factory({ ref: docRef })
+
+    await promise.value
+    // NOTE: why does toEqual fail here?
+    expect(data.value).toMatchObject({
+      id: docRef.id,
+      a: { name: 'a', id: aRef.id },
+    })
+  })
+
   it('unsubscribes from a ref if it is replaced', async () => {
     const docRef = await addDoc(listOfRefs, { a: aRef })
     const { data, pending, promise } = factory({ ref: docRef })

tests/firestore/utils.spec.ts

@@ -4,6 +4,7 @@ import {
   extractRefs,
   firestoreDefaultConverter,
 } from '../../src/firestore/utils'
+import { globalFirestoreOptions } from '../../src'
 import { setupFirestoreRefs } from '../utils'
 
 describe('Firestore and Database utils', () => {
@@ -56,11 +57,12 @@ describe('Firestore and Database utils', () => {
         ref: docRef,
       },
       undefined,
-      {}
+      {},
+      globalFirestoreOptions
     )
     expect(noRefsDoc.ref).toBe(docRef.path)
     expect(refs).toEqual({
-      ref: docRef,
+      ref: expect.objectContaining({ id: docRef.id }),
     })
   })
 
@@ -72,7 +74,8 @@ describe('Firestore and Database utils', () => {
         bar: d,
       },
       undefined,
-      {}
+      {},
+      globalFirestoreOptions
     )
     expect(doc.foo).toBe(1)
     expect(doc.bar).toBe(d)
@@ -87,7 +90,8 @@ describe('Firestore and Database utils', () => {
         bar: d,
       },
       undefined,
-      {}
+      {},
+      globalFirestoreOptions
     )
     expect(doc.foo).toBe(1)
     expect(doc.bar).toBe(d)
@@ -102,7 +106,8 @@ describe('Firestore and Database utils', () => {
         bar: d,
       },
       undefined,
-      {}
+      {},
+      globalFirestoreOptions
     )
     expect(doc.foo).toBe(1)
     expect(doc.bar).toBe(d)
@@ -117,11 +122,12 @@ describe('Firestore and Database utils', () => {
         },
       },
       undefined,
-      {}
+      {},
+      globalFirestoreOptions
     )
     expect(noRefsDoc.obj.ref).toBe(docRef.path)
     expect(refs).toEqual({
-      'obj.ref': docRef,
+      'obj.ref': expect.objectContaining({ id: docRef.id }),
     })
   })
 
@@ -134,7 +140,8 @@ describe('Firestore and Database utils', () => {
         },
       },
       undefined,
-      {}
+      {},
+      globalFirestoreOptions
     )
     expect(noRefsDoc).toEqual({
       a: null,
@@ -155,11 +162,12 @@ describe('Firestore and Database utils', () => {
         },
       },
       undefined,
-      {}
+      {},
+      globalFirestoreOptions
     )
     expect(noRefsDoc.obj.nested.ref).toBe(docRef.path)
     expect(refs).toEqual({
-      'obj.nested.ref': docRef,
+      'obj.nested.ref': expect.objectContaining({ id: docRef.id }),
     })
   })
 
@@ -170,15 +178,16 @@ describe('Firestore and Database utils', () => {
         arr: [docRef, docRef2, docRef],
       },
       undefined,
-      {}
+      {},
+      globalFirestoreOptions
     )
     expect(noRefsDoc.arr[0]).toBe(docRef.path)
     expect(noRefsDoc.arr[1]).toBe(docRef2.path)
     expect(noRefsDoc.arr[2]).toBe(docRef.path)
     expect(refs).toEqual({
-      'arr.0': docRef,
-      'arr.1': docRef2,
-      'arr.2': docRef,
+      'arr.0': expect.objectContaining({ id: docRef.id }),
+      'arr.1': expect.objectContaining({ id: docRef2.id }),
+      'arr.2': expect.objectContaining({ id: docRef.id }),
     })
   })
 
@@ -188,7 +197,12 @@ describe('Firestore and Database utils', () => {
       value: 'foo',
       enumerable: false,
     })
-    const [noRefsDoc, refs] = extractRefs(obj, undefined, {})
+    const [noRefsDoc, refs] = extractRefs(
+      obj,
+      undefined,
+      {},
+      globalFirestoreOptions
+    )
     expect(Object.getOwnPropertyDescriptor(noRefsDoc, 'bar')).toEqual({
       value: 'foo',
       enumerable: false,