Skip to content

Commit c9b0879

Browse files
posvaposva
committed
refactor(core): optimize extractRefs
1 parent 8e5f918 commit c9b0879

File tree

4 files changed

+134
-87
lines changed

4 files changed

+134
-87
lines changed

‎packages/@posva/vuefire-core/__tests__/firestore/refs-collections.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ describe('refs in collections', () => {
7373
await c.update({ isC: true })
7474

7575
await collection.add({ ref: c })
76-
// NOTE wait for refs to update
76+
// wait for refs to update
7777
await delay(5)
7878

7979
expect(vm.items).toEqual([

‎packages/@posva/vuefire-core/__tests__/firestore/utils.spec.ts

Lines changed: 57 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ describe('Firestore utils', () => {
5151
})
5252

5353
it('extract refs from document', () => {
54-
const [noRefsDoc, refs] = extractRefs(doc.data())
54+
const [noRefsDoc, refs] = extractRefs(doc.data(), undefined, {})
5555
expect(noRefsDoc.ref).toBe(docRef.path)
5656
expect(refs).toEqual({
5757
ref: docRef,
@@ -60,56 +60,76 @@ describe('Firestore utils', () => {
6060

6161
it('leave Date objects alone when extracting refs', () => {
6262
const d = new Date()
63-
const [doc, refs] = extractRefs({
64-
foo: 1,
65-
bar: d,
66-
})
63+
const [doc, refs] = extractRefs(
64+
{
65+
foo: 1,
66+
bar: d,
67+
},
68+
undefined,
69+
{}
70+
)
6771
expect(doc.foo).toBe(1)
6872
expect(doc.bar).toBe(d)
6973
expect(refs).toEqual({})
7074
})
7175

7276
it('leave Timestamps objects alone when extracting refs', () => {
7377
const d = new Timestamp(10, 10)
74-
const [doc, refs] = extractRefs({
75-
foo: 1,
76-
bar: d,
77-
})
78+
const [doc, refs] = extractRefs(
79+
{
80+
foo: 1,
81+
bar: d,
82+
},
83+
undefined,
84+
{}
85+
)
7886
expect(doc.foo).toBe(1)
7987
expect(doc.bar).toBe(d)
8088
expect(refs).toEqual({})
8189
})
8290

8391
it('leave GeoPoint objects alone when extracting refs', () => {
8492
const d = new GeoPoint(2, 48)
85-
const [doc, refs] = extractRefs({
86-
foo: 1,
87-
bar: d,
88-
})
93+
const [doc, refs] = extractRefs(
94+
{
95+
foo: 1,
96+
bar: d,
97+
},
98+
undefined,
99+
{}
100+
)
89101
expect(doc.foo).toBe(1)
90102
expect(doc.bar).toBe(d)
91103
expect(refs).toEqual({})
92104
})
93105

94106
it('extract object nested refs from document', () => {
95-
const [noRefsDoc, refs] = extractRefs({
96-
obj: {
97-
ref: docRef,
107+
const [noRefsDoc, refs] = extractRefs(
108+
{
109+
obj: {
110+
ref: docRef,
111+
},
98112
},
99-
})
113+
undefined,
114+
{}
115+
)
100116
expect(noRefsDoc.obj.ref).toBe(docRef.path)
101117
expect(refs).toEqual({
102118
'obj.ref': docRef,
103119
})
104120
})
105121

106122
it('works with null', () => {
107-
const [noRefsDoc, refs] = extractRefs({
108-
a: null,
109-
nested: {
123+
const [noRefsDoc, refs] = extractRefs(
124+
{
110125
a: null,
126+
nested: {
127+
a: null,
128+
},
111129
},
112-
})
130+
undefined,
131+
{}
132+
)
113133
expect(noRefsDoc).toEqual({
114134
a: null,
115135
nested: {
@@ -120,13 +140,17 @@ describe('Firestore utils', () => {
120140
})
121141

122142
it('extract deep object nested refs from document', () => {
123-
const [noRefsDoc, refs] = extractRefs({
124-
obj: {
125-
nested: {
126-
ref: docRef,
143+
const [noRefsDoc, refs] = extractRefs(
144+
{
145+
obj: {
146+
nested: {
147+
ref: docRef,
148+
},
127149
},
128150
},
129-
})
151+
undefined,
152+
{}
153+
)
130154
expect(noRefsDoc.obj.nested.ref).toBe(docRef.path)
131155
expect(refs).toEqual({
132156
'obj.nested.ref': docRef,
@@ -140,9 +164,13 @@ describe('Firestore utils', () => {
140164
data: {},
141165
index: 0,
142166
})
143-
const [noRefsDoc, refs] = extractRefs({
144-
arr: [docRef, docRef2, docRef],
145-
})
167+
const [noRefsDoc, refs] = extractRefs(
168+
{
169+
arr: [docRef, docRef2, docRef],
170+
},
171+
undefined,
172+
{}
173+
)
146174
expect(noRefsDoc.arr[0]).toBe(docRef.path)
147175
expect(noRefsDoc.arr[1]).toBe(docRef2.path)
148176
expect(noRefsDoc.arr[2]).toBe(docRef.path)

‎packages/@posva/vuefire-core/src/firestore/index.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ interface FirestoreSubscription {
2222
unsub: () => void
2323
// Firestore unique key eg: items/12
2424
path: string
25+
// TODO: can return null?
2526
data: () => firestore.DocumentData
2627
// // path inside the object to access the data items.3
2728
// key: string
@@ -35,6 +36,7 @@ function unsubscribeAll(subs: Record<string, FirestoreSubscription>) {
3536

3637
interface UpdateDataFromDocumentSnapshot {
3738
readonly snapshot: firestore.DocumentSnapshot
39+
// path in target -> FirestoreSubscription
3840
subs: Record<string, FirestoreSubscription>
3941
target: CommonBindOptionsParameter['vm']
4042
path: string
@@ -49,9 +51,7 @@ function updateDataFromDocumentSnapshot(
4951
) {
5052
// TODO: maybe we should options.serialize the snapshot here
5153
const [data, refs] = extractRefs(snapshot, walkGet(target, path), subs)
52-
// NOTE use ops
5354
ops.set(target, path, data)
54-
// walkSet(target, path, data)
5555
subscribeToRefs(
5656
{
5757
subs,
@@ -228,7 +228,7 @@ export function bindCollection(
228228
const oldData = array[oldIndex]
229229
const [data, refs] = extractRefs(snapshot, oldData, subs)
230230
// only move things around after extracting refs
231-
arraySubs.splice(oldIndex, 1)
231+
// only move things around after extracting refs
232232
arraySubs.splice(newIndex, 0, subs)
233233
ops.remove(array, oldIndex)
234234
ops.add(array, newIndex, data)

‎packages/@posva/vuefire-core/src/firestore/utils.ts

Lines changed: 73 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
import { firestore } from 'firebase'
2-
import { isTimestamp, isObject, isDocumentRef } from '../shared'
2+
import { isTimestamp, isObject, isDocumentRef, TODO } from '../shared'
33

44
export type FirestoreReference =
55
| firestore.Query
66
| firestore.DocumentReference
77
| firestore.CollectionReference
88

9-
export function createSnapshot(doc: firestore.DocumentSnapshot) {
9+
// TODO: fix type not to be any
10+
export function createSnapshot(doc: firestore.DocumentSnapshot): TODO {
1011
// TODO: it should create a deep copy instead because otherwise we will modify internal data
1112
// defaults everything to false, so no need to set
1213
return Object.defineProperty(doc.data() || {}, 'id', { value: doc.id })
@@ -16,62 +17,80 @@ export type FirestoreSerializer = typeof createSnapshot
1617

1718
export function extractRefs(
1819
doc: firestore.DocumentData,
19-
oldDoc: firestore.DocumentData = {},
20-
subs: Record<string, { path: string; data: () => firestore.DocumentData }> = {},
21-
path = '',
22-
result: [firestore.DocumentData, Record<string, firestore.DocumentReference>] = [{}, {}]
20+
oldDoc: firestore.DocumentData | void,
21+
subs: Record<string, { path: string; data: () => firestore.DocumentData }>
2322
): [firestore.DocumentData, Record<string, firestore.DocumentReference>] {
24-
// must be set here because walkGet can return null or undefined
25-
oldDoc = oldDoc || {}
26-
const [data, refs] = result
27-
// Add all properties that are not enumerable (not visible in the for loop)
28-
// getOwnPropertyDescriptors does not exist on IE
29-
// Object.getOwnPropertyNames(doc).forEach(propertyName => {
30-
// const descriptor = Object.getOwnPropertyDescriptor(doc, propertyName)
31-
// if (descriptor && !descriptor.enumerable) {
32-
// Object.defineProperty(data, propertyName, descriptor)
33-
// }
34-
// })
35-
const idDescriptor = Object.getOwnPropertyDescriptor(doc, 'id')
36-
if (idDescriptor && !idDescriptor.enumerable) {
37-
Object.defineProperty(data, 'id', idDescriptor)
38-
}
23+
const dataAndRefs: [firestore.DocumentData, Record<string, firestore.DocumentReference>] = [
24+
{},
25+
{},
26+
]
27+
28+
const subsByPath = Object.keys(subs).reduce((resultSubs, subKey) => {
29+
const sub = subs[subKey]
30+
resultSubs[sub.path] = sub.data()
31+
return resultSubs
32+
}, {} as Record<string, firestore.DocumentData>)
33+
34+
function recursiveExtract(
35+
doc: firestore.DocumentData,
36+
oldDoc: firestore.DocumentData | void,
37+
path: string,
38+
result: [firestore.DocumentData, Record<string, firestore.DocumentReference>]
39+
): void {
40+
// make it easier to later on access the value
41+
oldDoc = oldDoc || {}
42+
const [data, refs] = result
43+
// Add all properties that are not enumerable (not visible in the for loop)
44+
// getOwnPropertyDescriptors does not exist on IE
45+
// Object.getOwnPropertyNames(doc).forEach(propertyName => {
46+
// const descriptor = Object.getOwnPropertyDescriptor(doc, propertyName)
47+
// if (descriptor && !descriptor.enumerable) {
48+
// Object.defineProperty(data, propertyName, descriptor)
49+
// }
50+
// })
51+
// TODO: add test for the code above and remove this one
52+
const idDescriptor = Object.getOwnPropertyDescriptor(doc, 'id')
53+
if (idDescriptor && !idDescriptor.enumerable) {
54+
Object.defineProperty(data, 'id', idDescriptor)
55+
}
3956

40-
for (const key in doc) {
41-
const ref = doc[key]
42-
if (isDocumentRef(ref)) {
43-
// allow values to be null (like non-existant refs)
44-
// TODO: better typing since this isObject shouldn't be necessary but it doesn't work
45-
data[key] = typeof oldDoc === 'object' && key in oldDoc ? oldDoc[key] : ref.path
46-
// TODO handle subpathes?
47-
refs[path + key] = ref
48-
} else if (Array.isArray(ref)) {
49-
data[key] = Array(ref.length)
50-
// fill existing refs into data but leave the rest empty
51-
for (let i = 0; i < ref.length; i++) {
52-
const newRef = ref[i]
53-
const existingSub =
54-
subs[Object.keys(subs).find(subName => subs[subName].path === newRef.path)!]
55-
if (existingSub) {
56-
data[key][i] = existingSub.data()
57+
// recursively traverse doc to copy values and extract references
58+
for (const key in doc) {
59+
const ref = doc[key]
60+
if (
61+
// primitives
62+
ref == null ||
63+
// Firestore < 4.13
64+
ref instanceof Date ||
65+
isTimestamp(ref) ||
66+
(ref.longitude && ref.latitude) // GeoPoint
67+
) {
68+
data[key] = ref
69+
} else if (isDocumentRef(ref)) {
70+
// allow values to be null (like non-existant refs)
71+
// TODO: better typing since this isObject shouldn't be necessary but it doesn't work
72+
data[key] = typeof oldDoc === 'object' && key in oldDoc ? oldDoc[key] : ref.path
73+
// TODO: handle subpathes?
74+
refs[path + key] = ref
75+
} else if (Array.isArray(ref)) {
76+
data[key] = Array(ref.length)
77+
// fill existing refs into data but leave the rest empty
78+
for (let i = 0; i < ref.length; i++) {
79+
const newRef = ref[i]
80+
if (newRef.path in subsByPath) data[key][i] = subsByPath[newRef.path]
5781
}
82+
// the oldArray is in this case the same array with holes
83+
recursiveExtract(ref, data[key], path + key + '.', [data[key], refs])
84+
} else if (isObject(ref)) {
85+
data[key] = {}
86+
recursiveExtract(ref, oldDoc[key], path + key + '.', [data[key], refs])
87+
} else {
88+
data[key] = ref
5889
}
59-
// the oldArray is in this case the same array with holes
60-
extractRefs(ref, data[key], subs, path + key + '.', [data[key], refs])
61-
} else if (
62-
ref == null ||
63-
// Firestore < 4.13
64-
ref instanceof Date ||
65-
isTimestamp(ref) ||
66-
(ref.longitude && ref.latitude) // GeoPoint
67-
) {
68-
data[key] = ref
69-
} else if (isObject(ref)) {
70-
data[key] = {}
71-
extractRefs(ref, oldDoc[key], subs, path + key + '.', [data[key], refs])
72-
} else {
73-
data[key] = ref
7490
}
7591
}
76-
return result
92+
93+
recursiveExtract(doc, oldDoc, '', dataAndRefs)
94+
95+
return dataAndRefs
7796
}

0 commit comments

Comments
 (0)