Optional Datastore/FirestoreClient #3382

schmidt-sebastian · 2020-07-09T23:30:58Z

This implements a proposal for optional dependencies discussed in the firebase-js-sdk chatroom. We currently have:

class LiteFirestore {
  datastore: Datastore;
}

class Firestore extends LiteFirestore {
  firestoreClient: FirestoreClient;
}

This means that any user of Firestore will pull in Datastore and FirestoreClient, even if they were to just use Timestamp or (more realistically) the new methods that only read from cache.

To solve this problem, I added a components.ts file for both the Lite SDK and the firestore-exp SDK that manages the relationship between Firestore and its components. Each Firestore instance has up to one component, and the Firestore class is responsible for its cleanup (via the removeMethods).

To make this worthwhile I also move the ComponentProvider initialization into components.ts, which has some impact on enableIndexedDbPersistence and clearIndexdDbPersistence.

The first commit contains the code changes, the second commit contains the updated dependency file (auto-generated).

changeset-bot · 2020-07-09T23:31:00Z

💥 No Changeset

Latest commit: 5f22a4e

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

google-oss-bot · 2020-07-10T03:46:08Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`8a631e2`)	Head (`5376744`)	Diff
browser	245 kB	246 kB	+144 B (+0.1%)
esm2017	193 kB	193 kB	+22 B (+0.0%)
main	469 kB	469 kB	+181 B (+0.0%)
module	243 kB	243 kB	+122 B (+0.1%)
react-native	193 kB	193 kB	+22 B (+0.0%)

@firebase/firestore/exp

Type Base (8a631e2) Head (5376744) Diff

browser 187 kB 188 kB +281 B (+0.1%)

main 511 kB 512 kB +1.09 kB (+0.2%)

react-native 187 kB 188 kB +282 B (+0.2%)
@firebase/firestore/lite

Type Base (8a631e2) Head (5376744) Diff

browser 68.0 kB 68.4 kB +395 B (+0.6%)

main 497 kB 498 kB +991 B (+0.2%)

react-native 68.1 kB 68.4 kB +390 B (+0.6%)

`@firebase/firestore/memory`

Type	Base (`8a631e2`)	Head (`5376744`)	Diff
browser	184 kB	184 kB	+144 B (+0.1%)
esm2017	145 kB	145 kB	+22 B (+0.0%)
main	344 kB	344 kB	+181 B (+0.1%)
module	182 kB	182 kB	+122 B (+0.1%)
react-native	145 kB	145 kB	+22 B (+0.0%)

`firebase`

Type	Base (`8a631e2`)	Head (`5376744`)	Diff
firebase-firestore.js	283 kB	284 kB	+119 B (+0.0%)
firebase-firestore.memory.js	224 kB	224 kB	+119 B (+0.1%)
firebase.js	817 kB	817 kB	+121 B (+0.0%)

Test Logs

Base (8a631e2e): https://github.com/firebase/firebase-js-sdk/actions/runs/181524280
Head (53767444): https://github.com/firebase/firebase-js-sdk/actions/runs/181659149

Pretty sure this is causes the CI failure in #3382

schmidt-sebastian · 2020-07-21T22:46:36Z

@rafikhan Do you have time to review this?

schmidt-sebastian · 2020-07-22T23:17:54Z

I updated the PR to use a more generic mechanism for component removal - see 2bac522. Note that for now only one component exists so the difference is minimal.

rafikhan

Minor comments around logging and factoring out functions. Otherwise LGTM.

rafikhan · 2020-07-23T17:40:45Z

packages/firestore/lite/src/api/components.ts

+    const settings = firestore._getSettings();
+    const databaseInfo = new DatabaseInfo(
+      firestore._databaseId,
+      firestore._persistenceKey,
+      settings.host ?? DEFAULT_HOST,
+      settings.ssl ?? DEFAULT_SSL,
+      /* forceLongPolling= */ false
+    );
+    const datastorePromise = newConnection(databaseInfo).then(connection => {
+      const serializer = newSerializer(databaseInfo.databaseId);
+      const datastore = newDatastore(firestore._credentials, serializer);
+      datastore.start(connection);
+      return datastore;
+    });
+    datastoreInstances.set(firestore, datastorePromise);


Add appropriate logging during instantiation and removal.

rafikhan · 2020-07-23T17:45:04Z

packages/firestore/exp/src/api/database.ts

+  if (firestoreImpl._initialized && !firestoreImpl._terminated) {
+    throw new FirestoreError(
+      Code.FAILED_PRECONDITION,
+      'Persistence can only be cleared before a Firestore instance is ' +
+        'initialized or after it is terminated.'
    );
+  }


Suggestion: Consider putting this into an assert method like assertIsNotInitialized().

Kept as is for now since this is the only callsite and the only method that I am aware of that has this contract (plus I would have to come up with a good name - assertIsNotInitialized is only 90% correct...)

rafikhan

LGTM

Make Datastore optional

d7dd84e

schmidt-sebastian force-pushed the mrschmidt/optional branch from bc64fd0 to d7dd84e Compare July 15, 2020 20:14

schmidt-sebastian changed the title ~~WIP Tree-Shakeable Datastore~~ Optional Datastore/FirestoreClient Jul 15, 2020

Update deps

45ea646

schmidt-sebastian requested a review from rafikhan July 15, 2020 20:35

schmidt-sebastian assigned rafikhan Jul 15, 2020

Merge

a78a71e

schmidt-sebastian added a commit that referenced this pull request Jul 20, 2020

Wait for all Promises in deleteApp()

97cc6a0

Pretty sure this is causes the CI failure in #3382

schmidt-sebastian mentioned this pull request Jul 20, 2020

Wait for all Promises in deleteApp() #3439

Merged

schmidt-sebastian added 5 commits July 20, 2020 09:51

Cleanup

7e48aee

Merge

640c787

Fix comment

9559642

Fix unit test

0dcdd71

Lint

8d57993

schmidt-sebastian added the api: firestore label Jul 21, 2020

Use generic removeComponents

2bac522

schmidt-sebastian force-pushed the mrschmidt/optional branch from 3b24b7a to 2bac522 Compare July 22, 2020 23:16

rafikhan requested changes Jul 23, 2020

View reviewed changes

rafikhan assigned schmidt-sebastian and unassigned rafikhan Jul 23, 2020

Add logging

81755fc

schmidt-sebastian force-pushed the mrschmidt/optional branch from cc3452f to 81755fc Compare July 23, 2020 20:05

schmidt-sebastian assigned rafikhan and unassigned schmidt-sebastian Jul 23, 2020

rafikhan approved these changes Jul 24, 2020

View reviewed changes

Merge

7a21dca

rafikhan assigned schmidt-sebastian and unassigned rafikhan Jul 24, 2020

Update tests

5f22a4e

schmidt-sebastian merged commit 1a630ba into master Jul 24, 2020

firebase locked and limited conversation to collaborators Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Optional Datastore/FirestoreClient #3382

Optional Datastore/FirestoreClient #3382

Uh oh!

schmidt-sebastian commented Jul 9, 2020 •

edited

Loading

Uh oh!

changeset-bot bot commented Jul 9, 2020 •

edited

Loading

Uh oh!

google-oss-bot commented Jul 10, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`

Uh oh!

schmidt-sebastian commented Jul 21, 2020

Uh oh!

schmidt-sebastian commented Jul 22, 2020

Uh oh!

rafikhan left a comment

Uh oh!

rafikhan Jul 23, 2020

Uh oh!

schmidt-sebastian Jul 23, 2020

Uh oh!

rafikhan Jul 23, 2020 •

edited

Loading

Uh oh!

schmidt-sebastian Jul 23, 2020

Uh oh!

rafikhan Jul 24, 2020

Uh oh!

rafikhan left a comment

Uh oh!

Uh oh!

Optional Datastore/FirestoreClient #3382

Optional Datastore/FirestoreClient #3382

Uh oh!

Conversation

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

Uh oh!

Uh oh!

changeset-bot bot commented Jul 9, 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 10, 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/lite

@firebase/firestore/memory

firebase

Test Logs

Uh oh!

schmidt-sebastian commented Jul 21, 2020

Uh oh!

schmidt-sebastian commented Jul 22, 2020

Uh oh!

rafikhan left a comment

Choose a reason for hiding this comment

Uh oh!

rafikhan Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

rafikhan Jul 23, 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!

schmidt-sebastian Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

rafikhan Jul 24, 2020

Choose a reason for hiding this comment

Uh oh!

rafikhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian commented Jul 9, 2020 •

edited

Loading

changeset-bot bot commented Jul 9, 2020 •

edited

Loading

google-oss-bot commented Jul 10, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`

rafikhan Jul 23, 2020 •

edited

Loading