Skip to content

Commit b6b261b

Browse files
wu-huiwu-hui
committed
More feedbacks.
1 parent 57a1c63 commit b6b261b

File tree

4 files changed

+71
-65
lines changed

4 files changed

+71
-65
lines changed

‎packages/firestore/src/api/bundle.ts

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@ export class LoadBundleTask
2424
implements
2525
firestore.LoadBundleTask,
2626
PromiseLike<firestore.LoadBundleTaskProgress> {
27-
private _progressObserver?: PartialObserver<firestore.LoadBundleTaskProgress>;
27+
private _progressObserver: PartialObserver<
28+
firestore.LoadBundleTaskProgress
29+
> = {};
2830
private _taskCompletionResolver = new Deferred<
2931
firestore.LoadBundleTaskProgress
3032
>();
@@ -67,8 +69,12 @@ export class LoadBundleTask
6769
* `LoadBundleTaskProgress` object.
6870
*/
6971
_completeWith(progress: firestore.LoadBundleTaskProgress): void {
72+
debugAssert(
73+
progress.taskState === 'Success',
74+
'Task is not completed with Success.'
75+
);
7076
this._updateProgress(progress);
71-
if (this._progressObserver && this._progressObserver.complete) {
77+
if (this._progressObserver.complete) {
7278
this._progressObserver.complete();
7379
}
7480

@@ -82,11 +88,11 @@ export class LoadBundleTask
8288
_failWith(error: Error): void {
8389
this._lastProgress.taskState = 'Error';
8490

85-
if (this._progressObserver && this._progressObserver.next) {
91+
if (this._progressObserver.next) {
8692
this._progressObserver.next(this._lastProgress);
8793
}
8894

89-
if (this._progressObserver && this._progressObserver.error) {
95+
if (this._progressObserver.error) {
9096
this._progressObserver.error(error);
9197
}
9298

@@ -99,12 +105,12 @@ export class LoadBundleTask
99105
*/
100106
_updateProgress(progress: firestore.LoadBundleTaskProgress): void {
101107
debugAssert(
102-
this._lastProgress.taskState !== 'Error',
103-
'Cannot update progress on a failed task'
108+
this._lastProgress.taskState === 'Running',
109+
'Cannot update progress on a completed or failed task'
104110
);
105111

106112
this._lastProgress = progress;
107-
if (this._progressObserver && this._progressObserver.next) {
113+
if (this._progressObserver.next) {
108114
this._progressObserver.next(progress);
109115
}
110116
}

‎packages/firestore/src/core/bundle.ts

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ export function bundleSuccessProgress(
140140
};
141141
}
142142

143-
export class LoadResult {
143+
export class BundleLoadResult {
144144
constructor(
145145
readonly progress: firestore.LoadBundleTaskProgress,
146146
readonly changedDocs?: MaybeDocumentMap
@@ -211,28 +211,23 @@ export class BundleLoader {
211211
/**
212212
* Update the progress to 'Success' and return the updated progress.
213213
*/
214-
async complete(): Promise<LoadResult> {
215-
const lastDocument =
216-
this.documents.length === 0
217-
? null
218-
: this.documents[this.documents.length - 1];
214+
async complete(): Promise<BundleLoadResult> {
219215
debugAssert(
220-
!!lastDocument ||
221-
!lastDocument!.metadata.exists ||
222-
(!!lastDocument!.metadata.exists && !!lastDocument!.document),
223-
'Bundled documents ends with a document metadata.'
216+
this.documents[this.documents.length - 1]?.metadata.exists !== true ||
217+
!!this.documents[this.documents.length - 1].document,
218+
'Bundled documents ends with a document metadata and missing document.'
224219
);
225220

226221
for (const q of this.queries) {
227222
await saveNamedQuery(this.localStore, q);
228223
}
229224

230-
let changedDocs;
231-
if (this.documents.length > 0) {
232-
changedDocs = await applyBundleDocuments(this.localStore, this.documents);
233-
}
225+
const changedDocs = await applyBundleDocuments(
226+
this.localStore,
227+
this.documents
228+
);
234229

235230
this.progress.taskState = 'Success';
236-
return new LoadResult({ ...this.progress }, changedDocs);
231+
return new BundleLoadResult({ ...this.progress }, changedDocs);
237232
}
238233
}

‎packages/firestore/src/core/firestore_client.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -533,7 +533,7 @@ export class FirestoreClient {
533533
this.asyncQueue.enqueueAndForget(async () => {
534534
loadBundle(this.syncEngine, reader, task);
535535
return task.catch(e => {
536-
logWarn(`Loading bundle failed with ${e}`);
536+
logWarn(LOG_TAG, `Loading bundle failed with ${e}`);
537537
});
538538
});
539539

‎packages/firestore/test/unit/specs/bundle_spec.test.ts

Lines changed: 47 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -67,31 +67,36 @@ function bundleWithDocument(testDoc: TestBundleDocument): string {
6767
describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
6868
specTest('Newer docs from bundles should overwrite cache', [], () => {
6969
const query1 = Query.atPath(path('collection'));
70-
const docA = doc('collection/a', 1000, { key: 'a' });
71-
const docAChanged = doc('collection/a', 2999, { key: 'b' });
70+
const docA = doc('collection/a', 1000, { value: 'a' });
71+
const docAChanged = doc('collection/a', 2999, { value: 'b' });
7272

7373
const bundleString = bundleWithDocument({
7474
key: docA.key,
7575
readTime: 3000,
7676
createTime: 1999,
7777
updateTime: 2999,
78-
content: { key: 'b' }
78+
content: { value: 'b' }
7979
});
8080

81-
return spec()
82-
.userListens(query1)
83-
.watchAcksFull(query1, 1000, docA)
84-
.expectEvents(query1, { added: [docA] })
85-
.loadBundle(bundleString)
86-
.expectEvents(query1, { modified: [docAChanged] });
81+
return (
82+
spec()
83+
.userListens(query1)
84+
.watchAcksFull(query1, 1000, docA)
85+
.expectEvents(query1, { added: [docA] })
86+
// TODO(b/160876443): This currently raises snapshots with
87+
// `fromCache=false` if users already listen to some queries and bundles
88+
// has newer version.
89+
.loadBundle(bundleString)
90+
.expectEvents(query1, { modified: [docAChanged] })
91+
);
8792
});
8893

8994
specTest(
90-
'Newer deleted docs from bundles should delete cache docs',
95+
'Newer deleted docs from bundles should delete cached docs',
9196
[],
9297
() => {
9398
const query1 = Query.atPath(path('collection'));
94-
const docA = doc('collection/a', 1000, { key: 'a' });
99+
const docA = doc('collection/a', 1000, { value: 'a' });
95100

96101
const bundleString = bundleWithDocument({
97102
key: docA.key,
@@ -109,7 +114,7 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
109114

110115
specTest('Older deleted docs from bundles should do nothing', [], () => {
111116
const query1 = Query.atPath(path('collection'));
112-
const docA = doc('collection/a', 1000, { key: 'a' });
117+
const docA = doc('collection/a', 1000, { value: 'a' });
113118

114119
const bundleString = bundleWithDocument({
115120
key: docA.key,
@@ -131,22 +136,22 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
131136
[],
132137
() => {
133138
const query = Query.atPath(path('collection'));
134-
const docA = doc('collection/a', 250, { key: 'a' });
139+
const docA = doc('collection/a', 250, { value: 'a' });
135140

136141
const bundleBeforeMutationAck = bundleWithDocument({
137142
key: docA.key,
138143
readTime: 500,
139144
createTime: 250,
140145
updateTime: 500,
141-
content: { key: 'b' }
146+
content: { value: 'b' }
142147
});
143148

144149
const bundleAfterMutationAck = bundleWithDocument({
145150
key: docA.key,
146151
readTime: 1001,
147152
createTime: 250,
148153
updateTime: 1001,
149-
content: { key: 'fromBundle' }
154+
content: { value: 'fromBundle' }
150155
});
151156
return (
152157
spec()
@@ -156,29 +161,29 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
156161
.userListens(query)
157162
.watchAcksFull(query, 250, docA)
158163
.expectEvents(query, {
159-
added: [doc('collection/a', 250, { key: 'a' })]
164+
added: [doc('collection/a', 250, { value: 'a' })]
160165
})
161-
.userPatches('collection/a', { key: 'patched' })
166+
.userPatches('collection/a', { value: 'patched' })
162167
.expectEvents(query, {
163168
modified: [
164169
doc(
165170
'collection/a',
166171
250,
167-
{ key: 'patched' },
172+
{ value: 'patched' },
168173
{ hasLocalMutations: true }
169174
)
170175
],
171176
hasPendingWrites: true
172177
})
173178
.writeAcks('collection/a', 1000)
174-
// loading bundleBeforeMutationAck will not raise snapshots, because it is before
175-
// the acknowledged mutation.
179+
// loading bundleBeforeMutationAck will not raise snapshots, because its
180+
// snapshot version is older than the acknowledged mutation.
176181
.loadBundle(bundleBeforeMutationAck)
177182
// loading bundleAfterMutationAck will raise a snapshot, because it is after
178183
// the acknowledged mutation.
179184
.loadBundle(bundleAfterMutationAck)
180185
.expectEvents(query, {
181-
modified: [doc('collection/a', 1001, { key: 'fromBundle' })]
186+
modified: [doc('collection/a', 1001, { value: 'fromBundle' })]
182187
})
183188
);
184189
}
@@ -189,14 +194,14 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
189194
[],
190195
() => {
191196
const query = Query.atPath(path('collection'));
192-
const docA = doc('collection/a', 250, { key: 'a' });
197+
const docA = doc('collection/a', 250, { value: 'a' });
193198

194199
const bundleString = bundleWithDocument({
195200
key: docA.key,
196201
readTime: 1001,
197202
createTime: 250,
198203
updateTime: 1001,
199-
content: { key: 'fromBundle' }
204+
content: { value: 'fromBundle' }
200205
});
201206

202207
return (
@@ -205,36 +210,36 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
205210
.userListens(query)
206211
.watchAcksFull(query, 250, docA)
207212
.expectEvents(query, {
208-
added: [doc('collection/a', 250, { key: 'a' })]
213+
added: [doc('collection/a', 250, { value: 'a' })]
209214
})
210-
.userPatches('collection/a', { key: 'patched' })
215+
.userPatches('collection/a', { value: 'patched' })
211216
.expectEvents(query, {
212217
modified: [
213218
doc(
214219
'collection/a',
215220
250,
216-
{ key: 'patched' },
221+
{ value: 'patched' },
217222
{ hasLocalMutations: true }
218223
)
219224
],
220225
hasPendingWrites: true
221226
})
222227
// Loading the bundle will not raise snapshots, because the
223-
// mutation is not acknowledged.
228+
// mutation has not been acknowledged.
224229
.loadBundle(bundleString)
225230
);
226231
}
227232
);
228233

229234
specTest('Newer docs from bundles might lead to limbo doc', [], () => {
230235
const query = Query.atPath(path('collection'));
231-
const docA = doc('collection/a', 1000, { key: 'a' });
236+
const docA = doc('collection/a', 1000, { value: 'a' });
232237
const bundleString1 = bundleWithDocument({
233238
key: docA.key,
234239
readTime: 500,
235240
createTime: 250,
236241
updateTime: 500,
237-
content: { key: 'b' }
242+
content: { value: 'b' }
238243
});
239244
const limboQuery = Query.atPath(docA.key.path);
240245

@@ -243,13 +248,13 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
243248
.withGCEnabled(false)
244249
.userListens(query)
245250
.watchAcksFull(query, 250)
246-
// Backend tells there is no such doc.
251+
// Backend tells is there is no such doc.
247252
.expectEvents(query, {})
248253
// Bundle tells otherwise, leads to limbo.
249254
.loadBundle(bundleString1)
250255
.expectLimboDocs(docA.key)
251256
.expectEvents(query, {
252-
added: [doc('collection/a', 500, { key: 'b' })],
257+
added: [doc('collection/a', 500, { value: 'b' })],
253258
fromCache: true
254259
})
255260
// .watchAcksFull(limboQuery, 1002, docA1)
@@ -259,7 +264,7 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
259264
.watchSnapshots(1002)
260265
.expectLimboDocs()
261266
.expectEvents(query, {
262-
removed: [doc('collection/a', 500, { key: 'b' })],
267+
removed: [doc('collection/a', 500, { value: 'b' })],
263268
fromCache: false
264269
})
265270
);
@@ -270,13 +275,13 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
270275
['multi-client'],
271276
() => {
272277
const query = Query.atPath(path('collection'));
273-
const docA = doc('collection/a', 250, { key: 'a' });
278+
const docA = doc('collection/a', 250, { value: 'a' });
274279
const bundleString1 = bundleWithDocument({
275280
key: docA.key,
276281
readTime: 500,
277282
createTime: 250,
278283
updateTime: 500,
279-
content: { key: 'b' }
284+
content: { value: 'b' }
280285
});
281286

282287
return client(0)
@@ -292,7 +297,7 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
292297
// TODO(wuandy): Loading from secondary client does not notify other
293298
// clients for now. We need to fix it and uncomment below.
294299
// .expectEvents(query, {
295-
// modified: [doc('collection/a', 500, { key: 'b' })],
300+
// modified: [doc('collection/a', 500, { value: 'b' })],
296301
// })
297302
}
298303
);
@@ -302,13 +307,13 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
302307
['multi-client'],
303308
() => {
304309
const query = Query.atPath(path('collection'));
305-
const docA = doc('collection/a', 250, { key: 'a' });
310+
const docA = doc('collection/a', 250, { value: 'a' });
306311
const bundleString1 = bundleWithDocument({
307312
key: docA.key,
308313
readTime: 500,
309314
createTime: 250,
310315
updateTime: 500,
311-
content: { key: 'b' }
316+
content: { value: 'b' }
312317
});
313318

314319
return client(0)
@@ -324,7 +329,7 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
324329
})
325330
.loadBundle(bundleString1)
326331
.expectEvents(query, {
327-
modified: [doc('collection/a', 500, { key: 'b' })]
332+
modified: [doc('collection/a', 500, { value: 'b' })]
328333
});
329334
}
330335
);
@@ -334,13 +339,13 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
334339
['multi-client'],
335340
() => {
336341
const query = Query.atPath(path('collection'));
337-
const docA = doc('collection/a', 250, { key: 'a' });
342+
const docA = doc('collection/a', 250, { value: 'a' });
338343
const bundleString1 = bundleWithDocument({
339344
key: docA.key,
340345
readTime: 500,
341346
createTime: 250,
342347
updateTime: 500,
343-
content: { key: 'b' }
348+
content: { value: 'b' }
344349
});
345350

346351
return client(0)
@@ -357,11 +362,11 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
357362
.client(0)
358363
.loadBundle(bundleString1)
359364
.expectEvents(query, {
360-
modified: [doc('collection/a', 500, { key: 'b' })]
365+
modified: [doc('collection/a', 500, { value: 'b' })]
361366
})
362367
.client(1)
363368
.expectEvents(query, {
364-
modified: [doc('collection/a', 500, { key: 'b' })]
369+
modified: [doc('collection/a', 500, { value: 'b' })]
365370
});
366371
}
367372
);

0 commit comments

Comments
 (0)