Bundles load from secondary tabs #3393

wu-hui · 2020-07-11T01:07:20Z

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

Read the contribution guidelines (CONTRIBUTING.md).
If this has been discussed in an issue, make sure to link to the issue here.
If not, go file an issue about this before creating a pull request to discuss.

Testing

Make sure all existing tests in the repository pass after your change.
If you fixed a bug or added a feature, add a new test to cover your code.

API Changes

At this time we cannot accept changes that affect the public API. If you'd like to help
us make Firebase APIs better, please propose your change in an issue so that we
can discuss it together.

# 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.

changeset-bot · 2020-07-11T01:07:23Z

💥 No Changeset

Latest commit: 5a4d0f2

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

# Conflicts: # integration/messaging/test/utils/openNewTab.js # packages/firestore-types/index.d.ts # 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/bundle_spec.test.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

google-oss-bot · 2020-07-11T16:56:00Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`2bfa185`)	Head (`0c062c6`)	Diff
browser	258 kB	258 kB	+384 B (+0.1%)
esm2017	201 kB	201 kB	+254 B (+0.1%)
main	514 kB	515 kB	+1.02 kB (+0.2%)
module	255 kB	255 kB	+384 B (+0.2%)
react-native	201 kB	201 kB	+254 B (+0.1%)

@firebase/firestore/exp

Type Base (2bfa185) Head (0c062c6) Diff

main 415 kB 416 kB +812 B (+0.2%)

`@firebase/firestore/memory`

Type	Base (`2bfa185`)	Head (`0c062c6`)	Diff
browser	195 kB	195 kB	+61 B (+0.0%)
esm2017	153 kB	153 kB	+28 B (+0.0%)
main	384 kB	384 kB	+248 B (+0.1%)
module	193 kB	193 kB	+61 B (+0.0%)
react-native	153 kB	153 kB	+28 B (+0.0%)

`firebase`

Type	Base (`2bfa185`)	Head (`0c062c6`)	Diff
firebase-firestore.js	295 kB	295 kB	+362 B (+0.1%)
firebase-firestore.memory.js	235 kB	235 kB	+60 B (+0.0%)
firebase.js	829 kB	829 kB	+362 B (+0.0%)

Test Logs

Base (2bfa1853): https://github.com/firebase/firebase-js-sdk/actions/runs/165681294
Head (0c062c6b): https://github.com/firebase/firebase-js-sdk/actions/runs/180237958

schmidt-sebastian · 2020-07-17T22:41:39Z

packages/firestore/src/core/sync_engine.ts

+  async synchronizeWithChangedDocuments(): Promise<void> {
+    const changes = await this.localStore.getNewDocumentChanges();
+    await this.emitNewSnapsAndNotifyLocalStore(changes);
+  }


This is the style of method where I would advocate to drop the await keyword and simply return the Promise. See here for the code overhead of using await in ES5: https://www.typescriptlang.org/play/?target=1#code/IYZwngdgxgBAZgV2gFwJYHsL3egFAShgG8AoGcmYAd2FWRggFMqYAFAJ3QFtURHcCMALwA+YgF98AbhLigA

Note that __awaiter and __generator are already part of our code base, so we are only looking at the overhead in foo.

Done, thanks for the explaination.

schmidt-sebastian · 2020-07-17T22:44:27Z

packages/firestore/src/platform/browser/dom.ts

+/**
+ * An instance of the Platform's 'TextEncoder' implementation.
+ */
+export function newTextEncoder(): TextEncoder {


This should only be in platform is there is some divergence between platforms. This doesn't seem to be the case?

yarn test:node complains there is no TextEncoder, it looks like it has to be imported from util.

Sad :/

Should we move this to the ./serializer.ts file though? It seems much more fitting.

Hmm, but it is not used in serializer.ts. The only place it is used in non-testing code is util/bundle_reader.ts, I think it makes slightly more sense to keep it here.

I mean to move it from packages/firestore/src/platform/browser/dom.ts to packages/firestore/src/platform/browser/serializer.ts. It has little to do with DOM, mostly because it exists in Node which doesn't have DOM.

Ah, sorry. Done.

schmidt-sebastian · 2020-07-17T22:44:49Z

packages/firestore/src/local/shared_client_state_schema.ts

+// The WebStorage prefix that plays as a event to indicate the remote documents
+// might have changed due to some actions secondary tabs did.
+// format of the key is:
+//     firestore_remote_documents_changed_<persistence_prefix>


Suggested change

// firestore_remote_documents_changed_<persistence_prefix>

// firestore_remote_documents_changed_<persistenceKey>

schmidt-sebastian · 2020-07-17T22:45:47Z

packages/firestore/src/local/shared_client_state_schema.ts

+// format of the key is:
+//     firestore_remote_documents_changed_<persistence_prefix>
+export const REMOTE_DOCUMENTS_CHANGED_KEY_PREFIX =
+  'firestore_remote_documents_changed';


So the remote documents change when queries get updated and even when mutations are committed, but this even only fires when a bundle is loaded. Should this be a bundle specific notification?

Please also update the key prefix. For sake of brevity, you could consider dropping "remote documents" altogether and just make it something like "bundle loaded".

schmidt-sebastian · 2020-07-17T22:47:02Z

packages/firestore/src/local/shared_client_state.ts

@@ -711,6 +719,10 @@ export class WebStorageSharedClientState implements SharedClientState {
    this.persistOnlineState(onlineState);
  }

+  remoteDocumentsChanged(): void {


notifyRemoteDocumentsChanged()? But also see my comment below: I think this should be bundle specific.

schmidt-sebastian · 2020-07-17T22:48:36Z

packages/firestore/src/core/sync_engine.ts

@@ -1412,7 +1417,11 @@ export function loadBundle(
  syncEngineImpl.assertSubscribed('loadBundle()');

  // eslint-disable-next-line @typescript-eslint/no-floating-promises
-  loadBundleImpl(syncEngineImpl, bundleReader, task);
+  loadBundleImpl(syncEngineImpl, bundleReader, task).then(() => {
+    if (!syncEngineImpl.isPrimaryClient) {


Is there a downside to invoking this on all tabs? Seems like we should issue this notification even on the primary tabs.

You are right, this can be called on all tabs.

schmidt-sebastian · 2020-07-17T22:49:00Z

packages/firestore/src/local/shared_client_state.ts

@@ -173,6 +174,8 @@ export interface SharedClientState {
  setOnlineState(onlineState: OnlineState): void;

  writeSequenceNumber(sequenceNumber: ListenSequenceNumber): void;
+
+  remoteDocumentsChanged(): void;


Please add JSDoc.

schmidt-sebastian · 2020-07-17T22:51:51Z

packages/firestore/src/local/shared_client_state_syncer.ts

@@ -49,4 +49,10 @@ export interface SharedClientStateSyncer {

  /** Returns the IDs of the clients that are currently active. */
  getActiveClients(): Promise<ClientId[]>;
+
+  /**
+   * Query for changed documents from remote document cache and raise snapshots


Suggested change

* Query for changed documents from remote document cache and raise snapshots

* Retrieves newly changed documents from remote document cache and raises snapshots

schmidt-sebastian · 2020-07-17T22:52:11Z

packages/firestore/src/platform/dom.ts

@@ -41,3 +41,30 @@ export function getDocument(): Document | null {
    return browser.getDocument();
  }
 }
+
+/**
+ * An instance of the Platform's 'TextEncoder' implementation or null


This never returns null.

schmidt-sebastian · 2020-07-22T16:56:05Z

packages/firestore/src/local/shared_client_state_schema.ts

+// format of the key is:
+//     firestore_remote_documents_changed_<persistence_prefix>
+export const REMOTE_DOCUMENTS_CHANGED_KEY_PREFIX =
+  'firestore_remote_documents_changed';


Please also update the key prefix. For sake of brevity, you could consider dropping "remote documents" altogether and just make it something like "bundle loaded".

schmidt-sebastian · 2020-07-22T16:57:30Z

packages/firestore/src/platform/browser/dom.ts

+/**
+ * An instance of the Platform's 'TextEncoder' implementation.
+ */
+export function newTextEncoder(): TextEncoder {


Sad :/

Should we move this to the ./serializer.ts file though? It seems much more fitting.

schmidt-sebastian · 2020-07-22T16:57:53Z

packages/firestore/src/util/bundle_reader.ts

@@ -77,6 +78,8 @@ export class BundleReader {
    /** The reader to read from underlying binary bundle data source. */
    private reader: ReadableStreamReader<Uint8Array>
  ) {
+    this.textDecoder = newTextDecoder();
+    debugAssert(!!this.textDecoder, 'Cannot create a valid text decoder.');


This would never fire, would it?

Right, fixed.

schmidt-sebastian · 2020-07-22T16:58:09Z

packages/firestore/src/util/bundle_reader.ts

@@ -131,7 +134,7 @@ export class BundleReader {
      return null;
    }

-    const lengthString = this.textDecoder.decode(lengthBuffer);
+    const lengthString = this.textDecoder!.decode(lengthBuffer);


schmidt-sebastian · 2020-07-22T16:58:16Z

packages/firestore/src/util/bundle_reader.ts

@@ -199,7 +202,7 @@ export class BundleReader {
      }
    }

-    const result = this.textDecoder.decode(this.buffer.slice(0, length));
+    const result = this.textDecoder!.decode(this.buffer.slice(0, length));


schmidt-sebastian · 2020-07-23T16:10:02Z

packages/firestore/src/local/shared_client_state.ts

@@ -532,6 +540,10 @@ export class WebStorageSharedClientState implements SharedClientState {

    this.onlineStateKey = createWebStorageOnlineStateKey(this.persistenceKey);

+    this.bundleChangedRemoteDocumentsKey = createRemoteDocumentsLoadFromBundleKey(


createRemoteDocumentsLoadFromBundleKey seems like it needs a name update now as well. createBundleLoadedKey?

For sake of brevity, I would also change bundleChangedRemoteDocumentsKey to bundleLoadedKey. I seems shorter and more specific.

schmidt-sebastian · 2020-07-23T16:12:47Z

packages/firestore/src/platform/browser/dom.ts

+/**
+ * An instance of the Platform's 'TextEncoder' implementation.
+ */
+export function newTextEncoder(): TextEncoder {


I mean to move it from packages/firestore/src/platform/browser/dom.ts to packages/firestore/src/platform/browser/serializer.ts. It has little to do with DOM, mostly because it exists in Node which doesn't have DOM.

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

Better spect test.

b364ab0

Load bundles from secondary clients should raise snapshots

2588f20

Support secondary to secondary.

93bc964

wu-hui requested a review from schmidt-sebastian July 16, 2020 00:52

wu-hui assigned schmidt-sebastian Jul 16, 2020

schmidt-sebastian reviewed Jul 17, 2020

View reviewed changes

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

Address comments.

e18493a

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

schmidt-sebastian reviewed Jul 22, 2020

View reviewed changes

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

Address more comments.

d175a14

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

schmidt-sebastian approved these changes Jul 23, 2020

View reviewed changes

Move newTextEncoder to serializer.ts

5a4d0f2

wu-hui merged commit 9bfb995 into wuandy/Bundles Jul 23, 2020

firebase locked and limited conversation to collaborators Aug 23, 2020

	// firestore_remote_documents_changed_<persistence_prefix>
	// firestore_remote_documents_changed_<persistenceKey>

	* Query for changed documents from remote document cache and raise snapshots
	* Retrieves newly changed documents from remote document cache and raises snapshots

		@@ -532,6 +540,10 @@ export class WebStorageSharedClientState implements SharedClientState {

		this.onlineStateKey = createWebStorageOnlineStateKey(this.persistenceKey);

		this.bundleChangedRemoteDocumentsKey = createRemoteDocumentsLoadFromBundleKey(

Bundles load from secondary tabs #3393

Bundles load from secondary tabs #3393

Uh oh!

Conversation

wu-hui commented Jul 11, 2020

Discussion

Testing

API Changes

Uh oh!

changeset-bot bot commented Jul 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💥 No Changeset

Uh oh!

google-oss-bot commented Jul 11, 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

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

changeset-bot bot commented Jul 11, 2020 •

edited

Loading

google-oss-bot commented Jul 11, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/memory`

`firebase`