Implement BundleCache for IDB and memory. #3170

wu-hui · 2020-06-05T15:16:20Z

No description provided.

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

schmidt-sebastian

This looks great! Some nits and one general question: Do we have a story for how to validate the actual JSON contents? We may want to validate the bundle data that we read from the user before writing it to persistence.

schmidt-sebastian · 2020-06-08T21:15:43Z

packages/firestore/src/core/bundle.ts

+export interface Bundle {
+  readonly id: string;
+  readonly version: number;
+  // When the saved bundle is built from the server SDKs.


Can you point out in the documentation that this is to SnapshotVersion.MIN if the above is not true? In its current form, the comment raises the question why the type is not nullable.

I am not sure what do you mean here, sorry!

Something like:

Set to the snapshot version of the bundle if created by the Server SDKs. Otherwise set to SnapshotVersion.MIN.

schmidt-sebastian · 2020-06-08T21:16:21Z

packages/firestore/src/local/bundle_cache.ts

+  ): PersistencePromise<Bundle | undefined>;
+
+  /**
+   * Saves a `BundleMetadata` from a bundle into local storage, using its id as the persistent key.


Nit: Wrap at 80 characters.

schmidt-sebastian · 2020-06-08T21:17:29Z

packages/firestore/src/local/indexeddb_bundle_cache.ts

+      .get(bundleId)
+      .next(bundle => {
+        if (bundle) {
+          return this.serializer.fromDbBundle(bundle!);


You should be able to drop the bang.

schmidt-sebastian · 2020-06-08T21:17:34Z

packages/firestore/src/local/indexeddb_bundle_cache.ts

+      .get(queryName)
+      .next(query => {
+        if (query) {
+          return this.serializer.fromDbNamedQuery(query!);


You should be able to drop the bang.

schmidt-sebastian · 2020-06-08T21:23:03Z

packages/firestore/src/local/local_serializer.ts

+      version: metadata.version!,
+      createTime: this.remoteSerializer.fromVersion(metadata.createTime!)
+    };
+  }


Ideally, we should not add any more code to the main SDK for "optional" features. Do you mind exploring what it would entail to make these functions free-standing? They would then look like:

function fromBundleMetadata(serializer: RemoteSerializer, metadata: bundleProto.BundleMetadata): Bundle { return { id: metadata.id!, version: metadata.version!, createTime: serializer.fromVersion(metadata.createTime!) }; }

If you have to make a ton of state public, you can look at what we did here:

firebase-js-sdk/packages/firestore/src/remote/datastore.ts

Line 51 in 5cf39ab

class DatastoreImpl extends Datastore {

DatastoreImp has public state, but in itself is private.

wu-hui

As discussed offline, validation will be added in future PRs, when I implement bundle loading.

wu-hui · 2020-06-10T17:30:45Z

packages/firestore/src/local/bundle_cache.ts

+  ): PersistencePromise<Bundle | undefined>;
+
+  /**
+   * Saves a `BundleMetadata` from a bundle into local storage, using its id as the persistent key.


wu-hui · 2020-06-10T17:31:10Z

packages/firestore/src/local/indexeddb_bundle_cache.ts

+      .get(bundleId)
+      .next(bundle => {
+        if (bundle) {
+          return this.serializer.fromDbBundle(bundle!);


wu-hui · 2020-06-10T17:31:26Z

packages/firestore/src/local/indexeddb_bundle_cache.ts

+      .get(queryName)
+      .next(query => {
+        if (query) {
+          return this.serializer.fromDbNamedQuery(query!);


wu-hui · 2020-06-10T17:58:42Z

packages/firestore/src/local/local_serializer.ts

+      version: metadata.version!,
+      createTime: this.remoteSerializer.fromVersion(metadata.createTime!)
+    };
+  }


wu-hui · 2020-06-10T18:36:05Z

packages/firestore/src/core/bundle.ts

+export interface Bundle {
+  readonly id: string;
+  readonly version: number;
+  // When the saved bundle is built from the server SDKs.


I am not sure what do you mean here, sorry!

schmidt-sebastian

Some final nits, but looks great. Thanks for the updates!

schmidt-sebastian · 2020-06-12T01:33:30Z

packages/firestore/src/core/bundle.ts

+export interface Bundle {
+  readonly id: string;
+  readonly version: number;
+  // When the saved bundle is built from the server SDKs.


Something like:

Set to the snapshot version of the bundle if created by the Server SDKs. Otherwise set to SnapshotVersion.MIN.

schmidt-sebastian · 2020-06-12T01:37:12Z

packages/firestore/src/platform/platform.ts

+   * @param source The data source to use.
+   * @param bytesPerRead How many bytes each `read()` from the returned reader
+   *        will read. It is ignored if the passed in source does not provide
+   *        such control(example: ReadableStream).


Missing space before (.

Your indent also seems slightly off. We usually indent by four spaces (not 6), but I do see some counterexamples in our sources (both 6 and 0 exists, but 6 is an outlier).

schmidt-sebastian · 2020-06-12T01:37:42Z

packages/firestore/src/platform_browser/browser_platform.ts

    }
    throw new Error(
-      'Source of `toByteStreamReader` has to be Uint8Array, ArrayBuffer or ReadableStream'
+      'Source of `toByteStreamReader` has to be a ArrayBuffer or ReadableStream'


schmidt-sebastian · 2020-06-12T01:38:50Z

packages/firestore/src/core/bundle.ts

+}
+
+/**
+ * Encodes a `BundledQuery` from bundle proto to a Query object.


s/from bundle/from a bundle/

schmidt-sebastian · 2020-06-12T03:42:41Z

packages/firestore/src/core/bundle.ts

+export interface NamedQuery {
+  readonly name: string;
+  readonly query: Query;
+  // When the results for this query are read to the saved bundle.


/** The time at which the results for this query were read. */

schmidt-sebastian · 2020-06-12T03:44:33Z

packages/firestore/src/core/bundle.ts

+  bundledQuery: bundleProto.BundledQuery
+): Query {
+  const query = serializer.remoteSerializer.convertQueryTargetToQuery({
+    parent: bundledQuery.parent!,


Did you file a bug to validate the bundle contents? As part of this we need to validate that the parent matches the Firestore project that we are loading the SDK into.

b/158846981

schmidt-sebastian · 2020-06-12T03:45:25Z

packages/firestore/src/core/bundle.ts

+  return {
+    id: dbBundle.bundleId,
+    createTime: serializer.fromDbTimestamp(dbBundle.createTime),
+    version: dbBundle.version


What is our versioning strategy? I wonder if we should reject any bundle with a version greater than 1. If so, please file a bug.

b/158845787

schmidt-sebastian · 2020-06-12T03:49:27Z

packages/firestore/src/core/bundle.ts

+}
+
+/** Encodes a BundleMetadata to a DbBundle. */
+export function toDbBundle(


I don't have an actual opinion, but I am a bit surprised that you moved this function out of the serializer file. I think it makes sense that even the free-standing functions are next to the main serializer, but I can also see why you moved them to. Just wanted to know whether you considered this?

No need to move them.

Moved them back. I think putting them here also makes sense, but I don't feel strongly anyways.

schmidt-sebastian · 2020-06-12T03:50:27Z

packages/firestore/test/unit/local/bundle_cache.test.ts

+import { expect } from 'chai';
+import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence';
+import { filter, orderBy, path } from '../../util/helpers';
+import * as persistenceHelpers from './persistence_test_helpers';


In general, I would suggest to avoid namespace imports (import * as ).

schmidt-sebastian · 2020-06-12T03:51:45Z

packages/firestore/test/unit/local/bundle_cache.test.ts

+  let persistence: IndexedDbPersistence;
+  beforeEach(async () => {
+    persistence = await persistenceHelpers.testIndexedDbPersistence({
+      synchronizeTabs: true


I would omit this setting. It poses questions as to why you added it here, but doesn't (yet) at much benefit - other than remind me that I forgot to reply to your question about multi-tab sync...

wu-hui added 13 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

wu-hui changed the base branch from wuandy/Bundles to wuandy/BundleReaderNode June 5, 2020 15:17

wu-hui requested a review from schmidt-sebastian June 5, 2020 15:17

wu-hui assigned schmidt-sebastian Jun 5, 2020

wu-hui added the api: firestore label Jun 5, 2020

schmidt-sebastian reviewed Jun 8, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui Jun 8, 2020

wu-hui added 6 commits June 9, 2020 21:04

Address comments and remove hacking.

de1d162

Drop custom reader type.

1775298

Merge branch 'wuandy/BundleReaderNode' into wuandy/BundlePersistence

f572812

Address comments.

2dde827

Remove validation and use assertion.

6eafb6f

Merge branch 'wuandy/BundleReaderNode' into wuandy/BundlePersistence

a09a67f

wu-hui changed the base branch from wuandy/BundleReaderNode to wuandy/Bundles June 10, 2020 18:34

wu-hui commented Jun 10, 2020

View reviewed changes

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

wu-hui changed the base branch from wuandy/Bundles to wuandy/BundleReaderNode June 11, 2020 00:56

wu-hui changed the base branch from wuandy/BundleReaderNode to wuandy/Bundles June 11, 2020 00:57

Merge branch 'wuandy/Bundles' into wuandy/BundlePersistence

5b152df

schmidt-sebastian approved these changes Jun 12, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jun 12, 2020

Address more comments.

06e3e77

wu-hui merged commit d3f0ab4 into wuandy/Bundles Jun 14, 2020

firebase locked and limited conversation to collaborators Jul 15, 2020

Implement BundleCache for IDB and memory. #3170

Implement BundleCache for IDB and memory. #3170

Uh oh!

Conversation

wu-hui commented Jun 5, 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!

schmidt-sebastian Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui 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!

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

schmidt-sebastian Jun 8, 2020 •

edited

Loading