Implement bundle loading. #3201

wu-hui · 2020-06-10T14:42:37Z

No description provided.

# Conflicts: # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/util/bundle.test.ts

Fix change buffer bug

Two TODO: 1. load from secondary tab. 2. don't fail async queue.

# Conflicts: # integration/messaging/test/utils/openNewTab.js # packages/firestore/src/core/bundle.ts # packages/firestore/src/core/component_provider.ts # packages/firestore/src/core/firestore_client.ts # packages/firestore/src/core/sync_engine.ts # packages/firestore/src/local/bundle_cache.ts # packages/firestore/src/local/indexeddb_bundle_cache.ts # packages/firestore/src/local/indexeddb_persistence.ts # packages/firestore/src/local/indexeddb_remote_document_cache.ts # packages/firestore/src/local/local_serializer.ts # packages/firestore/src/local/local_store.ts # packages/firestore/src/local/memory_bundle_cache.ts # packages/firestore/src/local/memory_remote_document_cache.ts # packages/firestore/src/local/remote_document_change_buffer.ts # packages/firestore/src/platform/platform.ts # packages/firestore/src/platform_browser/browser_platform.ts # packages/firestore/src/platform_node/node_platform.ts # packages/firestore/src/remote/serializer.ts # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/local/bundle_cache.test.ts # packages/firestore/test/unit/local/local_store.test.ts # packages/firestore/test/unit/local/persistence_test_helpers.ts # packages/firestore/test/unit/local/test_bundle_cache.ts # packages/firestore/test/unit/specs/spec_test_components.ts # packages/firestore/test/unit/specs/spec_test_runner.ts # packages/firestore/test/unit/util/bundle.test.ts # packages/firestore/test/util/helpers.ts # packages/firestore/test/util/test_platform.ts

googlebot · 2020-06-29T19:17:31Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

schmidt-sebastian

Thanks for this!

schmidt-sebastian · 2020-06-29T18:10:52Z

packages/firestore/src/core/bundle.ts

@@ -98,3 +108,247 @@ export class BundleConverter {
    return fromVersion(time);
  }
 }
+
+/**
+ * Returns a `LoadBundleTaskProgress` representing the first progress of


Suggested change

* Returns a `LoadBundleTaskProgress` representing the first progress of

* Returns a `LoadBundleTaskProgress`, representing the initial progress of

schmidt-sebastian · 2020-06-29T18:12:48Z

packages/firestore/src/core/bundle.ts

+}
+
+/* eslint-disable @typescript-eslint/no-explicit-any */
+export class LoadBundleTaskImpl implements firestore.LoadBundleTask {


Couple things:

This could probably just be a LoadBundleTask? Our other API classes don't use the "Impl" prefix.

This should also be move to src/api/

All members that are not part of the public API should be prefixed with an underscore.

If possible, all instances of any should be replaced with unknown.

schmidt-sebastian · 2020-06-29T18:16:07Z

packages/firestore/src/core/bundle.ts

+    progress?: firestore.LoadBundleTaskProgress
+  ) => any;
+
+  private promiseResolver = new Deferred<any>();


This is a very generic name. Is this the "completionResolver"?

schmidt-sebastian · 2020-06-29T18:17:19Z

packages/firestore/src/core/bundle.ts

+   */
+  completeWith(progress: firestore.LoadBundleTaskProgress): void {
+    let result;
+    if (this.progressComplete) {


This sounds like a boolean state check, but it is far from it. We probably should rename the members that contain the user functions.

schmidt-sebastian · 2020-06-29T18:19:43Z

packages/firestore/src/core/bundle.ts

+    onFulfilled?: (a: firestore.LoadBundleTaskProgress) => any,
+    onRejected?: (a: Error) => any
+  ): Promise<any> {
+    this.promiseFulfilled = onFulfilled;


Do we need these members? Could we just call return this.promiseResolver.promise.then(onFullfilled, onRejected)?

We could, obviously I cannot wrap my mind around promises.

schmidt-sebastian · 2020-06-29T19:12:29Z

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

+          })
+          // Loading both bundles will not raise snapshots, because of the
+          // mutation is not acknowledged.
+          .loadBundle(bundleString1)


This bundle is ignored (as tested above), so we probably don't need to test it here. We could also add a small separate test that just verifies this code path.

Deleted it.

schmidt-sebastian · 2020-06-29T19:13:41Z

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

+            added: [docA]
+          })
+          .client(1)
+          // Bundle tells otherwise, leads to limbo resolution.


Please be more descriptive. What is the conflicting message in the bundle?

My bad, this is copied from above test, and should be deleted.

schmidt-sebastian · 2020-06-29T19:18:20Z

packages/firestore/test/unit/specs/spec_test_runner.ts

+    const task = new LoadBundleTaskImpl();
+    this.queue.enqueueAndForget(() => {
+      return loadBundle(this.syncEngine, reader, task);
+    });
+
+    await task;


Suggested change

const task = new LoadBundleTaskImpl();

this.queue.enqueueAndForget(() => {

return loadBundle(this.syncEngine, reader, task);

});

await task;

const task = new LoadBundleTaskImpl();

return this.queue.enqueue(() => loadBundle(this.syncEngine, reader, task));

schmidt-sebastian · 2020-06-29T19:21:33Z

packages/firestore/src/core/sync_engine.ts

+  syncEngine: SyncEngine,
+  bundleReader: BundleReader,
+  task: LoadBundleTaskImpl
+): Promise<void> {


Did you consider returning the LoadBundleTask instead of having it passed in? You are now mixing two async patterns in one callsite.

I agree it's awkward, the need for passing task as an argument comes from how firestore client needs to return the task itself, and loadBundle has to be called on async queue, i am not sure if returning a function from async queue would work there.

The need to return a promise here is for async queue to wait for loading to complete.

Do you have a more concrete sketch?

How about something like this: https://gist.github.com/schmidt-sebastian/22566fb61bfef994825bd24b45bcda02

Note that the diff is mostly whitespace, the change is actually pretty small. Basically, instead of returning the Promise here you can await the Task in the callsite.

schmidt-sebastian · 2020-06-29T19:24:26Z

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

+          // the acknowledged mutation.
+          .loadBundle(bundleString2)
+          .expectEvents(query, {
+            modified: [doc('collection/a', 1001, { key: 'fromBundle' })]


Ideally, this should also be fromCache.

google-oss-bot · 2020-06-29T19:27:41Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`3a78d7c`)	Head (`a6740c8`)	Diff
browser	250 kB	258 kB	+7.97 kB (+3.2%)
esm2017	195 kB	201 kB	+5.32 kB (+2.7%)
main	498 kB	512 kB	+14.1 kB (+2.8%)
module	248 kB	256 kB	+7.74 kB (+3.1%)

@firebase/firestore/exp

Type Base (3a78d7c) Head (a6740c8) Diff

main 277 kB 288 kB +10.9 kB (+3.9%)

`@firebase/firestore/memory`

Type	Base (`3a78d7c`)	Head (`a6740c8`)	Diff
browser	188 kB	196 kB	+8.10 kB (+4.3%)
esm2017	147 kB	153 kB	+5.46 kB (+3.7%)
main	368 kB	382 kB	+14.4 kB (+3.9%)
module	187 kB	194 kB	+7.86 kB (+4.2%)

`firebase`

Type	Base (`3a78d7c`)	Head (`a6740c8`)	Diff
firebase-firestore.js	289 kB	296 kB	+7.54 kB (+2.6%)
firebase-firestore.memory.js	228 kB	236 kB	+7.65 kB (+3.4%)
firebase.js	822 kB	829 kB	+7.56 kB (+0.9%)

Test Logs

Base (3a78d7ca): https://github.com/firebase/firebase-js-sdk/actions/runs/151812664
Head (a6740c86): https://github.com/firebase/firebase-js-sdk/actions/runs/165206333

schmidt-sebastian · 2020-07-07T17:09:58Z

packages/firestore/src/core/sync_engine.ts

+  syncEngine: SyncEngine,
+  bundleReader: BundleReader,
+  task: LoadBundleTaskImpl
+): Promise<void> {


How about something like this: https://gist.github.com/schmidt-sebastian/22566fb61bfef994825bd24b45bcda02

Note that the diff is mostly whitespace, the change is actually pretty small. Basically, instead of returning the Promise here you can await the Task in the callsite.

schmidt-sebastian · 2020-07-07T17:11:00Z

packages/firestore-types/index.d.ts

+    next?: (progress: LoadBundleTaskProgress) => any,
+    error?: (error: Error) => any,
+    complete?: () => void
+  ): Promise<void>;


Shouldn't this be 'void'? What does it mean for the onProgess callback to return a Promise?

schmidt-sebastian · 2020-07-07T17:11:31Z

packages/firestore-types/index.d.ts

+  then(
+    onFulfilled?: (a: LoadBundleTaskProgress) => any,
+    onRejected?: (a: Error) => any
+  ): Promise<any>;


Suggested change

then(

onFulfilled?: (a: LoadBundleTaskProgress) => any,

onRejected?: (a: Error) => any

): Promise<any>;

then<T,R>(

onFulfilled?: (a: LoadBundleTaskProgress) => T|PromiseLike<T>,

onRejected?: (a: Error) => R|PromiseLike<R>

): Promise<T|R>;

The ES5 definition for these types is:

interface Promise<T> { then<TResult1 = T, TResult2 = never>(onfulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | undefined | null, onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | undefined | null): Promise<TResult1 | TResult2>; catch<TResult = never>(onrejected?: ((reason: any) => TResult | PromiseLike<TResult>) | undefined | null): Promise<T | TResult>; }

Nice, thanks!

schmidt-sebastian · 2020-07-07T17:14:37Z

packages/firestore-types/index.d.ts

+    onRejected?: (a: Error) => any
+  ): Promise<any>;
+
+  catch(onRejected: (a: Error) => any): Promise<any>;


Suggested change

catch(onRejected: (a: Error) => any): Promise<any>;

catch(onRejected: (a: Error) => R|PromiseLike<R>): Promise<R>;

Done. Although it seems it should return a Promise<R|Progress> in this case.

schmidt-sebastian · 2020-07-07T17:15:54Z

packages/firestore/src/api/bundle.ts

+    this._userProgressHandler = next;
+    this._userProgressErrorHandler = error;
+    this._userProgressCompleteHandler = complete;
+    return this._progressResolver.promise;


I think we don't need to return this promise here.

schmidt-sebastian · 2020-07-07T19:14:58Z

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

+  readTime: TestSnapshotVersion;
+  createTime?: TestSnapshotVersion;
+  updateTime?: TestSnapshotVersion;
+  content?: api.ApiClientObjectMap<Value>;


The existing spec tests use the "user facing types" rather than the Value proto to encode the types. This means that we would store { foo: 1 } rather than { foo: { integerValue: 1 }}. I think that you will find that following this pattern will greatly simplify replaying these tests on Android and iOS.

schmidt-sebastian · 2020-07-07T19:15:27Z

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

+      .expectEvents(query1, { modified: [docAChanged], fromCache: true });
+  });
+
+  specTest('Newer deleted docs from bundles should delete cache', [], () => {


Suggested change

specTest('Newer deleted docs from bundles should delete cache', [], () => {

specTest('Newer deleted docs from bundles should delete cached docs', [], () => {

schmidt-sebastian · 2020-07-07T19:15:59Z

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

+  });
+
+  specTest(
+    'Newer docs from bundles should raise snapshot only when watch catches up with acknowledged writes',


Suggested change

'Newer docs from bundles should raise snapshot only when watch catches up with acknowledged writes',

'Newer docs from bundles should raise snapshot only when Watch catches up with acknowledged writes',

schmidt-sebastian · 2020-07-07T19:16:32Z

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

+      });
+      return (
+        spec()
+          .withGCEnabled(false)


Can you file a bug to define the behavior bundles when withGCEnabled is not set to false? Note that this flag is spec test only and that this not actually a mode that users can enable with memory persistence.

schmidt-sebastian · 2020-07-07T19:18:03Z

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

+          added: [doc('collection/a', 500, { key: 'b' })],
+          fromCache: true
+        })
+        .expectLimboDocs(docA.key)


Can you expand this test: We should verify that when the Limbo doc is resolved we go back to fromCache: false.

schmidt-sebastian

Next round. This is 98% done!

schmidt-sebastian · 2020-07-10T17:57:13Z

packages/firestore/src/api/bundle.ts

+  implements
+    firestore.LoadBundleTask,
+    PromiseLike<firestore.LoadBundleTaskProgress> {
+  private _progressObserver?: PartialObserver<firestore.LoadBundleTaskProgress>;


Consider initializing this to an empty observer: {}.

schmidt-sebastian · 2020-07-10T18:03:50Z

packages/firestore/src/api/bundle.ts

+
+    if (this._progressObserver && this._progressObserver.next) {
+      this._progressObserver.next(this._lastProgress);
+    }


Please try to match this behavior:

firebase-js-sdk/packages/storage/src/task.ts

Line 626 in 0131e1f

private notifyObserver_(observer: Observer<UploadTaskSnapshot>): void {

Basically, next doesn't get called for the Complete of Error state.

As discussed, we would keep current behaviour because these progress updates can be considered part of the progress update series.

schmidt-sebastian · 2020-07-10T18:04:42Z

packages/firestore/src/api/bundle.ts

+   * `LoadBundleTaskProgress` object.
+   */
+  _completeWith(progress: firestore.LoadBundleTaskProgress): void {
+    this._updateProgress(progress);


Do we not need to set the taskState here (like you did below)?

The call sites set this up. I added an assert instead.

schmidt-sebastian · 2020-07-10T18:05:39Z

packages/firestore/src/api/bundle.ts

+   */
+  _updateProgress(progress: firestore.LoadBundleTaskProgress): void {
+    debugAssert(
+      this._lastProgress.taskState !== 'Error',


You may want to add 'complete' to this check as well.

schmidt-sebastian · 2020-07-10T18:12:59Z

packages/firestore/src/core/bundle.ts

+    readonly progress: firestore.LoadBundleTaskProgress,
+    readonly changedDocs?: MaybeDocumentMap
+  ) {}
+}


Since this is an exported class, it should have a less generic name.

schmidt-sebastian · 2020-07-10T18:37:16Z

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

+            hasPendingWrites: true
+          })
+          .writeAcks('collection/a', 1000)
+          // loading bundleBeforeMutationAck will not raise snapshots, because it is before


s/it is before/its snapshot version is older than/

schmidt-sebastian · 2020-07-10T18:38:11Z

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

+        readTime: 1001,
+        createTime: 250,
+        updateTime: 1001,
+        content: { key: 'fromBundle' }


s/key/value since the key is 'a'. You need to update all document contents.

schmidt-sebastian · 2020-07-10T18:38:48Z

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

+            hasPendingWrites: true
+          })
+          // Loading the bundle will not raise snapshots, because the
+          // mutation is not acknowledged.


s/is not/has not been/

schmidt-sebastian · 2020-07-10T18:39:16Z

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

+        .withGCEnabled(false)
+        .userListens(query)
+        .watchAcksFull(query, 250)
+        // Backend tells there is no such doc.


s/tells/tells us

schmidt-sebastian · 2020-07-10T18:41:26Z

packages/firestore/src/core/firestore_client.ts

+    this.asyncQueue.enqueueAndForget(async () => {
+      loadBundle(this.syncEngine, reader, task);
+      return task.catch(e => {
+        logWarn(`Loading bundle failed with ${e}`);


You should add a LOG_TAG (see most other log lines).

schmidt-sebastian · 2020-07-10T23:59:52Z

packages/firestore-types/index.d.ts

  INTERNAL: { delete: () => Promise<void> };
 }

+export interface LoadBundleTask {
+  onProgress(


BTW, should we include an overload that takes an Observer?

Yeah, it's actually in the API doc. I will add it in a followup PR.

schmidt-sebastian · 2020-07-11T00:00:47Z

packages/firestore/src/core/bundle.ts

+export class BundleLoadResult {
+  constructor(
+    readonly progress: firestore.LoadBundleTaskProgress,
+    readonly changedDocs?: MaybeDocumentMap


This doesn't need to be optional anymore, which I think leads to one more simplification.

wu-hui added 27 commits May 21, 2020 13:49

Renaming interfaces without leading I

9602712

Initial commit of bundle reading - for web only.

c5e783e

Tests only run when it is not Node.

5e7fb89

Fix redundant imports

1ee1615

Fix missing textencoder

18f0be1

Remove generator.

aa455bf

Support bundle reader for Node

78248cd

Fix rebase errors.

83160a1

Remote 'only'

24e10cb

Merge branch 'wuandy/Bundles' into wuandy/BundleReaderNode

9d6edc5

# Conflicts: # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/util/bundle.test.ts

Merge branch 'wuandy/Bundles' into wuandy/BundleReaderNode

4cbe608

# Conflicts: # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/util/bundle.test.ts

Added more comments, and more tests for Node.

4313e51

Implement BundleCache.

296cfc4

Add applyBundleDocuments to local store.

fff3d36

Add rest of bundle service to localstore

fb762de

Fix change buffer bug

Simplify change buffer get read time logic.

1ec4182

Fix lint errors

cd3ab7a

Add comments.

d991c75

Change localstore to check for newer bundle directly.

af097c5

temp checkin

e735e23

Implement without async/await

17ba434

Major code complete.

f808d8d

Integration tests added.

db1d864

Added spec tests.

979ffd9

Two TODO: 1. load from secondary tab. 2. don't fail async queue.

Add comments and move types to d.ts

f7ff495

Support loading string for real.

556a007

Makes sure SDK still works after loading bad bundles.

adf1504

wu-hui force-pushed the wuandy/BundleLoadProgress branch from d6fafba to adf1504 Compare June 11, 2020 15:09

wu-hui added 2 commits June 12, 2020 15:45

Better spect test.

b364ab0

schmidt-sebastian reviewed Jun 29, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui Jun 29, 2020

wu-hui added 4 commits June 29, 2020 16:24

Finally ready for initial review.

21d4d7c

Address comments batch 1

bf085ce

Fix bytesPerRead default

8fbdd3e

Snapshots only once in the end.

985b205

wu-hui assigned schmidt-sebastian Jul 7, 2020

schmidt-sebastian reviewed Jul 7, 2020

View reviewed changes

schmidt-sebastian removed their assignment Jul 7, 2020

wu-hui added 5 commits July 8, 2020 12:05

Temp addressing.

685624a

Apply bundle.ts patch

5576963

Change how task is passed.

85e3ac6

From promise<void> to void

2f639ae

Spec tests comments

57a1c63

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jul 9, 2020

schmidt-sebastian reviewed Jul 10, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jul 10, 2020

More feedbacks.

b6b261b

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jul 10, 2020

schmidt-sebastian approved these changes Jul 11, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jul 11, 2020

Even more feedbacks.

6094153

wu-hui merged commit 0bf9c73 into wuandy/Bundles Jul 11, 2020

firebase locked and limited conversation to collaborators Aug 11, 2020

	* Returns a `LoadBundleTaskProgress` representing the first progress of
	* Returns a `LoadBundleTaskProgress`, representing the initial progress of

-  then(
-    onFulfilled?: (a: LoadBundleTaskProgress) => any,
-    onRejected?: (a: Error) => any
-  ): Promise<any>;
+  then<T,R>(
+    onFulfilled?: (a: LoadBundleTaskProgress) => T|PromiseLike<T>,
+    onRejected?: (a: Error) => R|PromiseLike<R>
+  ): Promise<T|R>;

	catch(onRejected: (a: Error) => any): Promise<any>;
	catch(onRejected: (a: Error) => R\|PromiseLike<R>): Promise<R>;

	specTest('Newer deleted docs from bundles should delete cache', [], () => {
	specTest('Newer deleted docs from bundles should delete cached docs', [], () => {

	'Newer docs from bundles should raise snapshot only when watch catches up with acknowledged writes',
	'Newer docs from bundles should raise snapshot only when Watch catches up with acknowledged writes',

Implement bundle loading. #3201

Implement bundle loading. #3201

Uh oh!

Conversation

wu-hui commented Jun 10, 2020

Uh oh!

googlebot commented Jun 29, 2020

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/exp

@firebase/firestore/memory

firebase

Test Logs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

google-oss-bot commented Jun 29, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/memory`

`firebase`