Address some of the feedback

Author: schmidt-sebastian

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

@@ -37,9 +37,9 @@ import {
 } from '../../../src/core/component_provider';
 import {
   FirebaseFirestore as LiteFirestore,
-  FirestoreDatabase,
   Settings as LiteSettings
 } from '../../../lite/src/api/database';
+import { DatabaseId } from '../../../src/core/database_info';
 import { Code, FirestoreError } from '../../../src/util/error';
 import { Deferred } from '../../../src/util/promise';
 import { LRU_MINIMUM_CACHE_SIZE_BYTES } from '../../../src/local/lru_garbage_collector';
@@ -77,11 +77,12 @@ export class FirebaseFirestore
   _firestoreClient: FirestoreClient | undefined;
 
   constructor(
-    app: FirestoreDatabase | FirebaseApp,
+    databaseIdOrApp: DatabaseId | FirebaseApp,
     authProvider: Provider<FirebaseAuthInternalName>
   ) {
-    super(app, authProvider);
-    this._persistenceKey = 'name' in app ? app.name : '[DEFAULT]';
+    super(databaseIdOrApp, authProvider);
+    this._persistenceKey =
+      'name' in databaseIdOrApp ? databaseIdOrApp.name : '[DEFAULT]';
   }
 
   _terminate(): Promise<void> {

packages/firestore/index.console.ts

@@ -20,9 +20,10 @@ import {
   Firestore as FirestoreCompat,
   MemoryPersistenceProvider
 } from './src/api/database';
-import { FirestoreDatabase } from './lite/src/api/database';
 import { Provider } from '@firebase/component';
 import { FirebaseAuthInternalName } from '@firebase/auth-interop-types';
+import { DatabaseId } from './src/core/database_info';
+import { Code, FirestoreError } from './src/util/error';
 export {
   CollectionReference,
   DocumentReference,
@@ -35,16 +36,36 @@ export { FieldPath } from './src/api/field_path';
 export { FieldValue } from './src/compat/field_value';
 export { Timestamp } from './src/api/timestamp';
 
+export interface FirestoreDatabase {
+  projectId: string;
+  database?: string;
+}
+
 /** Firestore class that exposes the constructor expected by the Console. */
 export class Firestore extends FirestoreCompat {
   constructor(
-    databaseId: FirestoreDatabase,
+    firestoreDatabase: FirestoreDatabase,
     authProvider: Provider<FirebaseAuthInternalName>
   ) {
     super(
-      databaseId,
-      new FirestoreExp(databaseId, authProvider),
+      databaseIdFromFirestoreDatabase(firestoreDatabase),
+      new FirestoreExp(
+        databaseIdFromFirestoreDatabase(firestoreDatabase),
+        authProvider
+      ),
       new MemoryPersistenceProvider()
     );
   }
 }
+
+function databaseIdFromFirestoreDatabase(
+  firestoreDatabase: FirestoreDatabase
+): DatabaseId {
+  if (!firestoreDatabase.projectId) {
+    throw new FirestoreError(Code.INVALID_ARGUMENT, 'Must provide projectId');
+  }
+  return new DatabaseId(
+    firestoreDatabase.projectId,
+    firestoreDatabase.database
+  );
+}

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

@@ -47,15 +47,6 @@ declare module '@firebase/component' {
 const DEFAULT_HOST = 'firestore.googleapis.com';
 const DEFAULT_SSL = true;
 
-/**
- * Options that can be provided in the Firestore constructor when not using
- * Firebase (aka standalone mode).
- */
-export interface FirestoreDatabase {
-  projectId: string;
-  database?: string;
-}
-
 export interface Settings {
   host?: string;
   ssl?: boolean;
@@ -175,25 +166,16 @@ export class FirebaseFirestore implements _FirebaseService {
   private _app?: FirebaseApp;
 
   constructor(
-    databaseIdOrApp: FirebaseApp | FirestoreDatabase,
+    databaseIdOrApp: DatabaseId | FirebaseApp,
     authProvider: Provider<FirebaseAuthInternalName>
   ) {
-    if (typeof (databaseIdOrApp as FirebaseApp).options === 'object') {
+    if (databaseIdOrApp instanceof DatabaseId) {
+      this._databaseId = databaseIdOrApp;
+      this._credentials = new EmptyCredentialsProvider();
+    } else {
       this._app = databaseIdOrApp as FirebaseApp;
-      this._databaseId = FirebaseFirestore._databaseIdFromApp(
-        databaseIdOrApp as FirebaseApp
-      );
+      this._databaseId = databaseIdFromApp(databaseIdOrApp as FirebaseApp);
       this._credentials = new FirebaseCredentialsProvider(authProvider);
-    } else {
-      const external = databaseIdOrApp as FirestoreDatabase;
-      if (!external.projectId) {
-        throw new FirestoreError(
-          Code.INVALID_ARGUMENT,
-          'Must provide projectId'
-        );
-      }
-      this._databaseId = new DatabaseId(external.projectId, external.database);
-      this._credentials = new EmptyCredentialsProvider();
     }
   }
 
@@ -244,17 +226,6 @@ export class FirebaseFirestore implements _FirebaseService {
     return this._settings;
   }
 
-  private static _databaseIdFromApp(app: FirebaseApp): DatabaseId {
-    if (!Object.prototype.hasOwnProperty.apply(app.options, ['projectId'])) {
-      throw new FirestoreError(
-        Code.INVALID_ARGUMENT,
-        '"projectId" not provided in firebase.initializeApp.'
-      );
-    }
-
-    return new DatabaseId(app.options.projectId!);
-  }
-
   _delete(): Promise<void> {
     if (!this._terminateTask) {
       this._terminateTask = this._terminate();
@@ -275,6 +246,17 @@ export class FirebaseFirestore implements _FirebaseService {
   }
 }
 
+function databaseIdFromApp(app: FirebaseApp): DatabaseId {
+  if (!Object.prototype.hasOwnProperty.apply(app.options, ['projectId'])) {
+    throw new FirestoreError(
+      Code.INVALID_ARGUMENT,
+      '"projectId" not provided in firebase.initializeApp.'
+    );
+  }
+
+  return new DatabaseId(app.options.projectId!);
+}
+
 /**
  * Initializes a new instance of Cloud Firestore with the provided settings.
  * Can only be called before any other functions, including

packages/firestore/src/api/database.ts

@@ -139,10 +139,7 @@ import {
   WriteBatch as PublicWriteBatch
 } from '@firebase/firestore-types';
 import { newUserDataReader } from '../../lite/src/api/reference';
-import {
-  FirestoreDatabase,
-  makeDatabaseInfo
-} from '../../lite/src/api/database';
+import { makeDatabaseInfo } from '../../lite/src/api/database';
 import { DEFAULT_HOST } from '../../lite/src/api/components';
 
 /**
@@ -227,13 +224,13 @@ export class Firestore
   implements PublicFirestore, FirebaseService {
   _appCompat?: FirebaseApp;
   constructor(
-    databaseIdOrApp: FirestoreDatabase | FirebaseApp,
+    databaseIdOrApp: DatabaseId | FirebaseApp,
     delegate: ExpFirebaseFirestore,
     private _persistenceProvider: PersistenceProvider
   ) {
     super(delegate);
 
-    if (typeof (databaseIdOrApp as FirebaseApp).options === 'object') {
+    if (!(databaseIdOrApp instanceof DatabaseId)) {
       this._appCompat = databaseIdOrApp as FirebaseApp;
     }
   }

packages/firestore/src/core/database_info.ts

@@ -63,11 +63,4 @@ export class DatabaseId {
       other.database === this.database
     );
   }
-
-  compareTo(other: DatabaseId): number {
-    return (
-      primitiveComparator(this.projectId, other.projectId) ||
-      primitiveComparator(this.database, other.database)
-    );
-  }
 }