Internal implementation of multiDb #1949

tom-andersen · 2022-10-21T16:18:27Z

No description provided.

wu-hui · 2022-10-21T18:26:59Z

please take package-lock.json out of this PR. Otherwise LGTM.

tom-andersen · 2022-10-21T19:52:28Z

please take package-lock.json out of this PR. Otherwise LGTM.

Done

lahirumaramba

Thank you @tom-andersen ! Overall LGTM!
I left a few comments and questions.

lahirumaramba · 2022-10-21T20:23:37Z

src/firestore/firestore-internal.ts

    this.appInternal = app;
  }

+  getDatabase(databaseId: string): Firestore {
+    let database = this.databases.get(databaseId);
+    if (database === undefined) {


Suggested change

if (database === undefined) {

if (typeof database === undefined) {

Or we could also do if (!this.databases.has(databaseId)) { just a suggestion really. I will leave it up to you. :)

Any of these solutions are valid. But I have a preference towards existing for a couple of reasons.

conciseness

performance (though only a small difference, calling has followed by get does consume more resources)

correctness (even if map has value, it could be 'undefined')

Just a note, using typeof will require putting quotes around undefined.

src/firestore/firestore-internal.ts

src/firestore/index.ts

lahirumaramba · 2022-10-21T20:46:00Z

src/firestore/index.ts

+  appOrDatabaseId?: App | string,
+  optionalDatabaseId?: string
+): Firestore {
+  const app: App = typeof appOrDatabaseId === 'object' ? appOrDatabaseId : getApp();


I think type of null is also object... we might want to check if app !== null as well. Alternatively, you can also move the implementations into the respective functions separately.

They would need to bypass Typescript type checking for this to happen, since appOrDatabaseId?: App | string.

According to this doc, we should not guard against invalid types.
https://g3doc.corp.google.com/firebase/g3doc/aip/3270.md?cl=head#input-validation

lahirumaramba · 2022-10-21T20:47:27Z

src/firestore/index.ts

+  const databaseId =
+    typeof appOrDatabaseId === 'string'
+      ? appOrDatabaseId
+      : optionalDatabaseId || '(default)';


Is '(default)' a const?

Yes. I have imported it from Server SDK (though it is not part of public API).

lahirumaramba · 2022-10-21T20:49:26Z

src/firestore/index.ts

+): Firestore {
+  const app: App = typeof appOrDatabaseId === 'object' ? appOrDatabaseId : getApp();
+  const databaseId =
+    typeof appOrDatabaseId === 'string'


Do we want to check if appOrDatabaseId is a non-empty string? We a validator util if this is needed validator.isNonEmptyString().

Good point. I think we should consider an empty string equivalent to the default database id.

lahirumaramba · 2022-10-21T20:50:28Z

test/unit/firestore/firestore.spec.ts

@@ -98,23 +98,25 @@ describe('Firestore', () => {
      it(`should throw given invalid app: ${ JSON.stringify(invalidApp) }`, () => {
        expect(() => {
          const firestoreAny: any = FirestoreService;
-          return new firestoreAny(invalidApp);
+          const firestoreService: FirestoreService = new firestoreAny(invalidApp);
+          return firestoreService.getDatabase('(default)')


Should we make this '(default)' a constant?

Imported from Server SDK

lahirumaramba · 2022-10-21T20:52:01Z

test/unit/firestore/firestore.spec.ts

@@ -158,19 +160,6 @@ describe('Firestore', () => {
    });
  });

-  describe('client', () => {


Could this be a potential breaking change in the public API?

It's a private variable of an internal class. No one should rely on this externally.

lahirumaramba · 2022-10-21T20:57:00Z

src/firestore/index.ts

+/**
+ * @param app
+ * @param databaseId
+ * @internal


@internal will hide this from both reference docs and autocomplete (it will remove the typescript annotation). Is that what we expect here? If we only want to hide from reference docs and would like to keep autocomplete we can use @alpha instead.

I feel safer to keep this completely hidden for the moment. We have a follow up task to expose API.

lahirumaramba

LGTM. Thanks!

tom-andersen force-pushed the tomandersen/multiDb branch 4 times, most recently from e65059b to 93c8c4f Compare October 21, 2022 16:59

tom-andersen marked this pull request as ready for review October 21, 2022 17:06

tom-andersen assigned wu-hui Oct 21, 2022

Internal implementation of multiDb

a14144d

tom-andersen force-pushed the tomandersen/multiDb branch from 93c8c4f to a14144d Compare October 21, 2022 19:28

lahirumaramba requested changes Oct 21, 2022

View reviewed changes

lahirumaramba reviewed Oct 21, 2022

View reviewed changes

lahirumaramba assigned tom-andersen Oct 21, 2022

Fixes from PR review

11570d1

tom-andersen requested a review from lahirumaramba October 24, 2022 17:40

Fixes from PR review

18ae1f5

tom-andersen requested a review from markarndt October 25, 2022 19:53

tom-andersen assigned markarndt Oct 25, 2022

lahirumaramba approved these changes Oct 26, 2022

View reviewed changes

tom-andersen merged commit 4d6b79e into master Oct 26, 2022

	if (database === undefined) {
	if (typeof database === undefined) {

Internal implementation of multiDb #1949

Internal implementation of multiDb #1949

Uh oh!

Conversation

tom-andersen commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wu-hui commented Oct 21, 2022

Uh oh!

tom-andersen commented Oct 21, 2022

Uh oh!

lahirumaramba 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!

Uh oh!

Uh oh!

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!

lahirumaramba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tom-andersen commented Oct 21, 2022 •

edited

Loading