Misc fixes for tree-shakeable API (#3358)

Author: schmidt-sebastian

.changeset/strong-geckos-peel.md

@@ -0,0 +1,2 @@
+---
+---

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

@@ -33,11 +33,18 @@ import {
   MemoryComponentProvider
 } from '../../../src/core/component_provider';
 
-import { Firestore as LiteFirestore } from '../../../lite/src/api/database';
+import {
+  DEFAULT_FORCE_LONG_POLLING,
+  DEFAULT_HOST,
+  DEFAULT_SSL,
+  Firestore as LiteFirestore
+} from '../../../lite/src/api/database';
 import { cast } from '../../../lite/src/api/util';
 import { Code, FirestoreError } from '../../../src/util/error';
 import { Deferred } from '../../../src/util/promise';
 import { LruParams } from '../../../src/local/lru_garbage_collector';
+import { CACHE_SIZE_UNLIMITED } from '../../../src/api/database';
+import { DatabaseInfo } from '../../../src/core/database_info';
 
 /**
  * The root reference to the Firestore database and the entry point for the
@@ -46,25 +53,27 @@ import { LruParams } from '../../../src/local/lru_garbage_collector';
 export class Firestore extends LiteFirestore
   implements firestore.FirebaseFirestore, _FirebaseService {
   private readonly _queue = new AsyncQueue();
+  private readonly _firestoreClient: FirestoreClient;
   private readonly _persistenceKey: string;
   private _componentProvider: ComponentProvider = new MemoryComponentProvider();
 
   // Assigned via _getFirestoreClient()
-  private _firestoreClientPromise?: Promise<FirestoreClient>;
+  private _deferredInitialization?: Promise<void>;
 
   protected _persistenceSettings: PersistenceSettings = { durable: false };
   // We override the Settings property of the Lite SDK since the full Firestore
   // SDK supports more settings.
   protected _settings?: firestore.Settings;
 
-  _terminated: boolean = false;
+  private _terminated: boolean = false;
 
   constructor(
     app: FirebaseApp,
     authProvider: Provider<FirebaseAuthInternalName>
   ) {
     super(app, authProvider);
     this._persistenceKey = app.name;
+    this._firestoreClient = new FirestoreClient(this._credentials, this._queue);
   }
 
   _getSettings(): firestore.Settings {
@@ -75,26 +84,29 @@ export class Firestore extends LiteFirestore
   }
 
   _getFirestoreClient(): Promise<FirestoreClient> {
-    if (!this._firestoreClientPromise) {
+    if (this._terminated) {
+      throw new FirestoreError(
+        Code.FAILED_PRECONDITION,
+        'The client has already been terminated.'
+      );
+    }
+
+    if (!this._deferredInitialization) {
       const settings = this._getSettings();
       const databaseInfo = this._makeDatabaseInfo(
         settings.host,
         settings.ssl,
         settings.experimentalForceLongPolling
       );
 
-      const firestoreClient = new FirestoreClient(
+      this._deferredInitialization = this._firestoreClient.start(
         databaseInfo,
-        this._credentials,
-        this._queue
+        this._componentProvider,
+        this._persistenceSettings
       );
-
-      this._firestoreClientPromise = firestoreClient
-        .start(this._componentProvider, this._persistenceSettings)
-        .then(() => firestoreClient);
     }
 
-    return this._firestoreClientPromise;
+    return this._deferredInitialization.then(() => this._firestoreClient);
   }
 
   // TODO(firestorexp): Factor out MultiTabComponentProvider and remove
@@ -103,11 +115,11 @@ export class Firestore extends LiteFirestore
     persistenceProvider: ComponentProvider,
     synchronizeTabs: boolean
   ): Promise<void> {
-    if (this._firestoreClientPromise) {
+    if (this._deferredInitialization) {
       throw new FirestoreError(
         Code.FAILED_PRECONDITION,
         'Firestore has already been started and persistence can no longer ' +
-          'be enabled. You can only call enable persistence before calling ' +
+          'be enabled. You can only enable persistence before calling ' +
           'any other methods on a Firestore object.'
       );
     }
@@ -131,10 +143,10 @@ export class Firestore extends LiteFirestore
   }
 
   _clearPersistence(): Promise<void> {
-    if (this._firestoreClientPromise !== undefined && !this._terminated) {
+    if (this._deferredInitialization !== undefined && !this._terminated) {
       throw new FirestoreError(
         Code.FAILED_PRECONDITION,
-        'Persistence can only be cleared before the Firestore instance is ' +
+        'Persistence can only be cleared before a Firestore instance is ' +
           'initialized or after it is terminated.'
       );
     }
@@ -156,6 +168,30 @@ export class Firestore extends LiteFirestore
     });
     return deferred.promise;
   }
+
+  protected _makeDatabaseInfo(
+    host?: string,
+    ssl?: boolean,
+    forceLongPolling?: boolean
+  ): DatabaseInfo {
+    return new DatabaseInfo(
+      this._databaseId,
+      this._persistenceKey,
+      host ?? DEFAULT_HOST,
+      ssl ?? DEFAULT_SSL,
+      forceLongPolling ?? DEFAULT_FORCE_LONG_POLLING
+    );
+  }
+
+  _terminate(): Promise<void> {
+    this._terminated = true;
+    if (this._deferredInitialization) {
+      return this._deferredInitialization.then(() =>
+        this._firestoreClient.terminate()
+      );
+    }
+    return Promise.resolve();
+  }
 }
 
 export function initializeFirestore(
@@ -166,6 +202,18 @@ export function initializeFirestore(
     app,
     'firestore-exp'
   ).getImmediate() as Firestore;
+
+  if (
+    settings.cacheSizeBytes !== undefined &&
+    settings.cacheSizeBytes !== CACHE_SIZE_UNLIMITED &&
+    settings.cacheSizeBytes < LruParams.MINIMUM_CACHE_SIZE_BYTES
+  ) {
+    throw new FirestoreError(
+      Code.INVALID_ARGUMENT,
+      `cacheSizeBytes must be at least ${LruParams.MINIMUM_CACHE_SIZE_BYTES}`
+    );
+  }
+
   firestore._configureClient(settings);
   return firestore;
 }
@@ -233,8 +281,5 @@ export function terminate(
 ): Promise<void> {
   _removeServiceInstance(firestore.app, 'firestore/lite');
   const firestoreImpl = cast(firestore, Firestore);
-  firestoreImpl._terminated = true;
-  return firestoreImpl
-    ._getFirestoreClient()
-    .then(firestoreClient => firestoreClient.terminate());
+  return firestoreImpl._terminate();
 }

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

@@ -50,7 +50,7 @@ import {
 } from '../../../lite/src/api/reference';
 import { Document } from '../../../src/model/document';
 import { DeleteMutation, Precondition } from '../../../src/model/mutation';
-import { FieldPath } from '../../../src/api/field_path';
+import { FieldPath } from '../../../lite/src/api/field_path';
 import {
   CompleteFn,
   ErrorFn,
@@ -262,7 +262,7 @@ export function addDoc<T>(
   data: T
 ): Promise<firestore.DocumentReference<T>> {
   const collRef = cast<CollectionReference<T>>(reference, CollectionReference);
-  const firestore = cast(collRef, Firestore);
+  const firestore = cast(collRef.firestore, Firestore);
   const docRef = doc(collRef);
 
   const convertedValue = applyFirestoreDataConverter(collRef._converter, data);
@@ -404,7 +404,7 @@ export function onSnapshot<T>(
       );
   } else {
     const query = cast<Query<T>>(ref, Query);
-    const firestore = cast(query, Firestore);
+    const firestore = cast(query.firestore, Firestore);
 
     const observer: PartialObserver<ViewSnapshot> = {
       next: snapshot => {

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

@@ -76,7 +76,7 @@ export class DocumentSnapshot<T = firestore.DocumentData>
         this.metadata,
         /* converter= */ null
       );
-      return this._converter.fromFirestore(snapshot);
+      return this._converter.fromFirestore(snapshot, options);
     } else {
       const userDataWriter = new UserDataWriter(
         this._firestoreImpl._databaseId,

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

@@ -39,9 +39,9 @@ import { cast } from './util';
 import { Settings } from '../../';
 
 // settings() defaults:
-const DEFAULT_HOST = 'firestore.googleapis.com';
-const DEFAULT_SSL = true;
-const DEFAULT_FORCE_LONG_POLLING = false; // Used by full SDK
+export const DEFAULT_HOST = 'firestore.googleapis.com';
+export const DEFAULT_SSL = true;
+export const DEFAULT_FORCE_LONG_POLLING = false; // Used by full SDK
 
 /**
  * The root reference to the Firestore Lite database.
@@ -101,17 +101,13 @@ export class Firestore
     return this._datastorePromise;
   }
 
-  protected _makeDatabaseInfo(
-    host?: string,
-    ssl?: boolean,
-    forceLongPolling?: boolean
-  ): DatabaseInfo {
+  protected _makeDatabaseInfo(host?: string, ssl?: boolean): DatabaseInfo {
     return new DatabaseInfo(
       this._databaseId,
       /* persistenceKey= */ 'unsupported',
       host ?? DEFAULT_HOST,
       ssl ?? DEFAULT_SSL,
-      forceLongPolling ?? DEFAULT_FORCE_LONG_POLLING
+      DEFAULT_FORCE_LONG_POLLING
     );
   }
 

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

@@ -63,7 +63,7 @@ class FieldValueDelegate extends FieldValue implements firestore.FieldValue {
 }
 
 export function deleteField(): firestore.FieldValue {
-  return new FieldValueDelegate(new DeleteFieldValueImpl('delete'));
+  return new FieldValueDelegate(new DeleteFieldValueImpl('deleteField'));
 }
 
 export function serverTimestamp(): firestore.FieldValue {

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

@@ -291,7 +291,7 @@ export function collection(
   parent: firestore.FirebaseFirestore | firestore.DocumentReference<unknown>,
   relativePath: string
 ): CollectionReference<firestore.DocumentData> {
-  validateArgType('doc', 'non-empty string', 2, relativePath);
+  validateArgType('collection', 'non-empty string', 2, relativePath);
   const path = ResourcePath.fromString(relativePath);
   if (parent instanceof Firestore) {
     validateCollectionPath(path);

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

@@ -24,7 +24,6 @@ import {
 import { Code, FirestoreError } from '../../../src/util/error';
 import { applyFirestoreDataConverter } from '../../../src/api/database';
 import {
-  DocumentKeyReference,
   parseSetData,
   parseUpdateData,
   parseUpdateVarargs,
@@ -165,7 +164,7 @@ export class WriteBatch implements firestore.WriteBatch {
 export function validateReference<T>(
   documentRef: firestore.DocumentReference<T>,
   firestore: Firestore
-): DocumentKeyReference<T> {
+): DocumentReference<T> {
   if (documentRef.firestore !== firestore) {
     throw new FirestoreError(
       Code.INVALID_ARGUMENT,

packages/firestore/src/api/database.ts

@@ -430,7 +430,8 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
     ) {
       throw new FirestoreError(
         Code.FAILED_PRECONDITION,
-        'Persistence cannot be cleared after this Firestore instance is initialized.'
+        'Persistence can only be cleared before a Firestore instance is ' +
+          'initialized or after it is terminated.'
       );
     }
 
@@ -515,13 +516,13 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
 
     const databaseInfo = this.makeDatabaseInfo();
 
-    this._firestoreClient = new FirestoreClient(
+    this._firestoreClient = new FirestoreClient(this._credentials, this._queue);
+
+    return this._firestoreClient.start(
       databaseInfo,
-      this._credentials,
-      this._queue
+      componentProvider,
+      persistenceSettings
     );
-
-    return this._firestoreClient.start(componentProvider, persistenceSettings);
   }
 
   private static databaseIdFromApp(app: FirebaseApp): DatabaseId {

packages/firestore/src/core/firestore_client.ts

@@ -81,6 +81,7 @@ export class FirestoreClient {
   // initialization completes before any other work is queued, we're cheating
   // with the types rather than littering the code with '!' or unnecessary
   // undefined checks.
+  private databaseInfo!: DatabaseInfo;
   private eventMgr!: EventManager;
   private persistence!: Persistence;
   private localStore!: LocalStore;
@@ -94,7 +95,6 @@ export class FirestoreClient {
   private readonly clientId = AutoId.newId();
 
   constructor(
-    private databaseInfo: DatabaseInfo,
     private credentials: CredentialsProvider,
     /**
      * Asynchronous queue responsible for all of our internal processing. When
@@ -135,6 +135,7 @@ export class FirestoreClient {
    * fallback succeeds we signal success to the async queue even though the
    * start() itself signals failure.
    *
+   * @param databaseInfo The connection information for the current instance.
    * @param componentProvider Provider that returns all core components.
    * @param persistenceSettings Settings object to configure offline
    *     persistence.
@@ -144,10 +145,14 @@ export class FirestoreClient {
    *     unconditionally resolved.
    */
   start(
+    databaseInfo: DatabaseInfo,
     componentProvider: ComponentProvider,
     persistenceSettings: PersistenceSettings
   ): Promise<void> {
     this.verifyNotTerminated();
+
+    this.databaseInfo = databaseInfo;
+
     // We defer our initialization until we get the current user from
     // setChangeListener(). We block the async queue until we got the initial
     // user and the initialization is completed. This will prevent any scheduled

packages/firestore/test/integration/api/database.test.ts

@@ -1149,7 +1149,8 @@ apiDescribe('Database', (persistence: boolean) => {
         await expect(
           firestore.clearPersistence()
         ).to.eventually.be.rejectedWith(
-          'Persistence cannot be cleared after this Firestore instance is initialized.'
+          'Persistence can only be cleared before a Firestore instance is ' +
+            'initialized or after it is terminated.'
         );
       });
     }