Skip to content

Commit

Permalink
fix(database, firestore): avoid id field name conflict (vuejs#1472)
Browse files Browse the repository at this point in the history
  • Loading branch information
tlserver committed Dec 4, 2023
1 parent 64308fb commit 795c500
Show file tree
Hide file tree
Showing 9 changed files with 78 additions and 48 deletions.
36 changes: 29 additions & 7 deletions src/database/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,21 @@ export function createRecordFromDatabaseSnapshot(

const value: unknown = snapshot.val()
return isObject(value)
? (Object.defineProperty(value, 'id', {
// allow destructuring without interfering without using the `id` property
value: snapshot.key,
? (Object.defineProperties(value, {
// allow destructuring without interfering without using the `.key` property
'.key': { value: snapshot.key },
'.priority': { value: snapshot.priority },
'.ref': { value: snapshot.ref },
'.size': { value: snapshot.size },
}) as VueDatabaseDocumentData<unknown>)
: {
// if the value is a primitive we can just return a regular object, it's easier to debug
// @ts-expect-error: $value doesn't exist
$value: value,
id: snapshot.key,
'.key': snapshot.key,
'.priority': snapshot.priority,
'.ref': snapshot.ref,
'.size': snapshot.size,
}
}

Expand All @@ -43,7 +49,7 @@ export function indexForKey(
key: string | null | number
): number {
for (let i = 0; i < array.length; i++) {
if (array[i].id === key) return i
if (array[i]['.key'] === key) return i
}

return -1
Expand All @@ -58,9 +64,25 @@ export type VueDatabaseDocumentData<T = unknown> =
| null
| (T & {
/**
* id of the document
* The key (last part of the path) of the location of this DataSnapshot.The last token in a Database location is
* considered its key. For example, "ada" is the key for the /users/ada/ node. Accessing the key on any
* DataSnapshot will return the key for the location that generated it. However, accessing the key on the root URL
* of a Database will return null.
*/
readonly id: string
readonly '.key': string
/**
* The priority value of the data in this DataSnapshot.Applications need not use priority but can order
* collections by ordinary properties.
*/
readonly '.priority': string
/**
* The location of this document data.
*/
readonly '.ref': string
/**
* The number of child properties of this document data.
*/
readonly '.size': string
})

/**
Expand Down
10 changes: 9 additions & 1 deletion src/firestore/useFirestoreRef.ts
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,15 @@ export type VueFirestoreDocumentData<T = DocumentData> =
/**
* id of the document
*/
readonly id: string
readonly '.id': string
/**
* Metadata about the DocumentSnapshot, including information about its source and local modifications.
*/
readonly '.metadata': string
/**
* The DocumentReference for the document
*/
readonly '.ref': string
})

export type VueFirestoreQueryData<T = DocumentData> = Array<
Expand Down
22 changes: 11 additions & 11 deletions src/firestore/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,17 @@ export const firestoreDefaultConverter: FirestoreDataConverter<VueFirestoreDocum
return data as DocumentData
},
fromFirestore(snapshot, options) {
return snapshot.exists()
? (Object.defineProperties(snapshot.data(options)!, {
id: { value: snapshot.id },
// TODO: check if worth adding or should be through an option
// It could also be an example in the docs about converters
// $meta: {
// value: snapshot.metadata,
// },
// $ref: { get: () => snapshot.ref },
}) as VueFirestoreDocumentData)
: null
if (snapshot.exists()) {
const data = snapshot.data(options)!
Object.defineProperties(data, {
'.id': { value: snapshot.id },
'.metadata': { value: snapshot.metadata },
'.ref': { value: snapshot.ref },
})
return data as VueFirestoreDocumentData
} else {
return null
}
},
}

Expand Down
12 changes: 6 additions & 6 deletions tests/database/list.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,9 @@ describe('Database lists', () => {
await promise.value

expect(wrapper.vm.list).toMatchObject([
{ $value: 'a', id: expect.any(String) },
{ $value: 'b', id: expect.any(String) },
{ $value: 'c', id: expect.any(String) },
{ $value: 'a', '.key': expect.any(String) },
{ $value: 'b', '.key': expect.any(String) },
{ $value: 'c', '.key': expect.any(String) },
])
})

Expand Down Expand Up @@ -169,8 +169,8 @@ describe('Database lists', () => {

const a = await push(listRef, { name: 'a' })
expect(wrapper.vm.list).toHaveLength(1)
expect(data.value[0].id).toBeTypeOf('string')
expect(data.value[0].id).toEqual(a.key)
expect(data.value[0]['.key']).toBeTypeOf('string')
expect(data.value[0]['.key']).toEqual(a.key)
})

it('unbinds when the component is unmounted', async () => {
Expand Down Expand Up @@ -397,7 +397,7 @@ describe('Database lists', () => {
useDatabaseList(databaseRef(db, 'todos'))
)
expectType<string | undefined>(
useDatabaseList(databaseRef(db, 'todos')).value?.[0]?.id
useDatabaseList(databaseRef(db, 'todos')).value?.[0]?.['.key']
)
expectType<Ref<VueDatabaseQueryData<number>>>(
useDatabaseList<number>(databaseRef(db, 'todos'))
Expand Down
4 changes: 2 additions & 2 deletions tests/database/objects.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ describe('Database objects', () => {

expect(wrapper.vm.item).toMatchObject({
$value: 24,
id: itemRef.key,
'.key': itemRef.key,
})
})

Expand Down Expand Up @@ -294,7 +294,7 @@ describe('Database objects', () => {
useDatabaseObject<{ name: string }>(databaseRef(db, 'todo'))
)
expectType<undefined | string>(
useDatabaseObject(databaseRef(db, 'todo')).value?.id
useDatabaseObject(databaseRef(db, 'todo')).value?.['.key']
)
expectType<Ref<number | null | undefined>>(
useDatabaseObject<number>(databaseRef(db, 'todo'))
Expand Down
20 changes: 10 additions & 10 deletions tests/firestore/collection.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ describe(

const a = await addDoc(listRef, { name: 'a' })
expect(wrapper.vm.list).toHaveLength(1)
expect(data.value[0].id).toBeTypeOf('string')
expect(data.value[0].id).toEqual(a.id)
expect(data.value[0]['.id']).toBeTypeOf('string')
expect(data.value[0]['.id']).toEqual(a.id)
})

it('unbinds when the component is unbound', async () => {
Expand Down Expand Up @@ -472,13 +472,14 @@ describe(

// Adds the id
// FIXME: this one is any but the test passes
expectType<string>(useCollection(collection(db, 'todos')).value[0].id)
expectType<string>(useCollection(collection(db, 'todos')).value[0]['.id'])
expectType<string>(
useCollection<TodoI>(collection(db, 'todos')).value[0].id
useCollection<TodoI>(collection(db, 'todos')).value[0]['.id']
)
expectType<string>(
useCollection<unknown>(collection(db, 'todos')).value[0].id
useCollection<unknown>(collection(db, 'todos')).value[0]['.id']
)
// @ts-expect-error: no id with custom converter
useCollection(
collection(db, 'todos').withConverter<TodoI, DocumentData>({
fromFirestore: (snapshot) => {
Expand All @@ -487,18 +488,17 @@ describe(
},
toFirestore: (todo) => todo,
})
// @ts-expect-error: no id with custom converter
).value[0].id
).value[0]['.id']

expectType<Ref<TodoI[]>>(useCollection<TodoI>(collection(db, 'todos')))
expectType<Ref<TodoI[]>>(
useCollection<TodoI>(collection(db, 'todos')).data
)
expectType<string>(
useCollection<TodoI>(collection(db, 'todos')).value.at(0)!.id
useCollection<TodoI>(collection(db, 'todos')).value.at(0)!['.id']
)
expectType<string>(
useCollection<TodoI>(collection(db, 'todos')).data.value.at(0)!.id
useCollection<TodoI>(collection(db, 'todos')).data.value.at(0)!['.id']
)
// @ts-expect-error: wrong type
expectType<Ref<string[]>>(useCollection<TodoI>(collection(db, 'todos')))
Expand All @@ -513,7 +513,7 @@ describe(
expectType<Ref<number[]>>(useCollection(refWithConverter))
expectType<Ref<number[]>>(useCollection(refWithConverter).data)
// @ts-expect-error: no id with converter
expectType<Ref<number[]>>(useCollection(refWithConverter).data.value.id)
expectType<Ref<number[]>>(useCollection(refWithConverter).data.value['.id'])
// @ts-expect-error
expectType<Ref<string[]>>(useCollection(refWithConverter))
})
Expand Down
14 changes: 7 additions & 7 deletions tests/firestore/document.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ describe(
const { itemRef, data } = factory()

await setDoc(itemRef, { name: 'a' })
expect(data.value!.id).toBe(itemRef.id)
expect(data.value!['.id']).toBe(itemRef.id)
})

it('sets pending while loading', async () => {
Expand Down Expand Up @@ -347,12 +347,13 @@ describe(

// Adds the id
// FIXME: this one is any but the test passes
expectType<string>(useDocument(doc(db, 'todos', '1')).value?.id)
expectType<string>(useDocument<TodoI>(doc(db, 'todos', '1')).value!.id)
expectType<string>(useDocument(doc(db, 'todos', '1')).value?.['.id'])
expectType<string>(useDocument<TodoI>(doc(db, 'todos', '1')).value!['.id'])
expectType<_Nullable<TodoI>>(
useDocument<TodoI>(doc(db, 'todos', '1')).value
)
expectType<string>(useDocument<unknown>(doc(db, 'todos', '1')).value!.id)
expectType<string>(useDocument<unknown>(doc(db, 'todos', '1')).value!['.id'])
// @ts-expect-error: no id with custom converter
useDocument(
doc(db, 'todos').withConverter<TodoI, DocumentData>({
fromFirestore: (snapshot) => {
Expand All @@ -361,8 +362,7 @@ describe(
},
toFirestore: (todo) => todo,
})
// @ts-expect-error: no id with custom converter
).value?.id
).value?.['.id']

expectType<Ref<number | null | undefined>>(useDocument<number>(itemRef))
expectType<Ref<number | null | undefined>>(
Expand All @@ -386,7 +386,7 @@ describe(
// @ts-expect-error: string is not assignable to number
expectType<Ref<string>>(useDocument(refWithConverter))
// @ts-expect-error: no id when a custom converter is used
useDocument(refWithConverter).value.id
useDocument(refWithConverter).value['.id']

// destructuring
expectType<Ref<DocumentData | null | undefined>>(
Expand Down
4 changes: 2 additions & 2 deletions tests/firestore/refs-in-documents.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ describe('Firestore refs in documents', async () => {
await promise.value
// NOTE: why does toEqual fail here?
expect(data.value).toMatchObject({
id: docRef.id,
a: { name: 'a', id: aRef.id },
'.id': docRef.id,
a: { name: 'a', '.id': aRef.id },
})
})

Expand Down
4 changes: 2 additions & 2 deletions tests/firestore/utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ describe('Firestore and Database utils', () => {

it('createSnapshot adds an id', async () => {
const snapshot = await addDocToCollection()
expect(snapshot.data()?.id).not.toBeFalsy()
expect(snapshot.data()?.['.id']).not.toBeFalsy()
})

it('id is not enumerable', async () => {
const snapshot = await addDocToCollection()
expect(Object.keys(snapshot.data() ?? {}).includes('id')).toBe(false)
expect(Object.keys(snapshot.data() ?? {}).includes('.id')).toBe(false)
})

it('contains all the data', async () => {
Expand Down

0 comments on commit 795c500

Please sign in to comment.