Review feedback

Author: schmidt-sebastian

integration/firestore/gulpfile.js

@@ -28,7 +28,7 @@ function clean() {
 }
 
 function isPersistenceEnabled() {
-  return process.env.USE_FIRESTORE_PERSISTENCE !== 'false';
+  return process.env.DISABLE_FIRESTORE_PERSISTENCE !== 'true';
 }
 
 function copyTests() {
@@ -72,9 +72,9 @@ function copyTests() {
          import '${firebaseFirestoreSdk}';
          
          if (typeof process === 'undefined') {
-           process = { env: { USE_FIRESTORE_PERSISTENCE: '${isPersistenceEnabled()}' } } as any;
+           process = { env: { DISABLE_FIRESTORE_PERSISTENCE: '${!isPersistenceEnabled()}' } } as any;
          } else {
-           process.env.USE_FIRESTORE_PERSISTENCE = '${isPersistenceEnabled()}';
+           process.env.DISABLE_FIRESTORE_PERSISTENCE = '${!isPersistenceEnabled()}';
          }
          `
       )

integration/firestore/package.json

@@ -4,14 +4,14 @@
   "private": true,
   "scripts": {
     "build:dependencies": "(cd ../../ ; yarn build)",
-    "build": "yarn build:dependencies; USE_FIRESTORE_PERSISTENCE=true gulp compile-tests",
-    "build:memory": "yarn build:dependencies; USE_FIRESTORE_PERSISTENCE=false gulp compile-tests",
+    "build": "yarn build:dependencies; gulp compile-tests",
+    "build:memory": "yarn build:dependencies; DISABLE_FIRESTORE_PERSISTENCE=true gulp compile-tests",
     "pretest": "yarn build:dependencies",
     "pretest:manual": "yarn build",
     "pretest:manual:memory": "yarn build:memory",
     "pretest:debug": "yarn build",
     "pretest:debug:memort": "yarn build:memory",
-    "test": "USE_FIRESTORE_PERSISTENCE=true gulp compile-tests; karma start --single-run; USE_FIRESTORE_PERSISTENCE=false gulp compile-tests; karma start --single-run;",
+    "test": "gulp compile-tests; karma start --single-run; DISABLE_FIRESTORE_PERSISTENCE=true gulp compile-tests; karma start --single-run;",
     "test:manual": "karma start --single-run",
     "test:manual:memory": "karma start --single-run",
     "test:debug": "karma start --auto-watch --browsers Chrome",

packages/firestore/index.memory.ts

@@ -1,6 +1,6 @@
 /**
  * @license
- * Copyright 2020 Google Inc.
+ * Copyright 2020 Google LLC
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,12 +20,14 @@ import * as types from '@firebase/firestore-types';
 import { FirebaseNamespace } from '@firebase/app-types';
 
 import { configureForFirebase } from './src/platform/config';
+import './register-module';
 import './src/platform_browser/browser_init';
 
 import { name, version } from './package.json';
+import {Firestore, PublicMemoryFirestore} from "./src/api/database";
 
 export function registerFirestore(instance: FirebaseNamespace): void {
-  configureForFirebase(instance);
+  configureForFirebase(instance, PublicMemoryFirestore, Firestore);
   instance.registerVersion(name, version);
 }
 

packages/firestore/index.node.memory.ts

@@ -1,6 +1,6 @@
 /**
  * @license
- * Copyright 2020 Google Inc.
+ * Copyright 2020 Google LLC
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,43 +16,19 @@
  */
 
 import firebase from '@firebase/app';
-import * as types from '@firebase/firestore-types';
 import { FirebaseNamespace } from '@firebase/app-types';
 
 import { configureForFirebase } from './src/platform/config';
-import './src/platform_browser/browser_init';
+import './register-module';
+import './src/platform_node/node_init';
 
 import { name, version } from './package.json';
+import {Firestore, PublicMemoryFirestore} from "./src/api/database";
 
 export function registerFirestore(instance: FirebaseNamespace): void {
-  configureForFirebase(instance);
+  configureForFirebase(instance, PublicMemoryFirestore, Firestore);
   instance.registerVersion(name, version);
 }
 
 registerFirestore(firebase);
 
-declare module '@firebase/app-types' {
-  interface FirebaseNamespace {
-    firestore?: {
-      (app?: FirebaseApp): types.FirebaseFirestore;
-      Blob: typeof types.Blob;
-      CollectionReference: typeof types.CollectionReference;
-      DocumentReference: typeof types.DocumentReference;
-      DocumentSnapshot: typeof types.DocumentSnapshot;
-      FieldPath: typeof types.FieldPath;
-      FieldValue: typeof types.FieldValue;
-      Firestore: typeof types.FirebaseFirestore;
-      GeoPoint: typeof types.GeoPoint;
-      Query: typeof types.Query;
-      QueryDocumentSnapshot: typeof types.QueryDocumentSnapshot;
-      QuerySnapshot: typeof types.QuerySnapshot;
-      Timestamp: typeof types.Timestamp;
-      Transaction: typeof types.Transaction;
-      WriteBatch: typeof types.WriteBatch;
-      setLogLevel: typeof types.setLogLevel;
-    };
-  }
-  interface FirebaseApp {
-    firestore?(): types.FirebaseFirestore;
-  }
-}

packages/firestore/index.node.ts

@@ -15,44 +15,21 @@
  * limitations under the License.
  */
 import firebase from '@firebase/app';
-import * as types from '@firebase/firestore-types';
 import { configureForFirebase } from './src/platform/config';
-import { registerFirestorePersistence } from './src/platform/persistence';
+import './register-module';
 import './src/platform_node/node_init';
 import { FirebaseNamespace } from '@firebase/app-types';
 
 import { name, version } from './package.json';
+import {
+  PersistenceFirestore,
+  PublicPersistenceFirestore
+} from "./src/api/persistence";
 
 export function registerFirestore(instance: FirebaseNamespace): void {
-  configureForFirebase(instance);
-  registerFirestorePersistence(instance);
+  configureForFirebase(instance, PublicPersistenceFirestore, PersistenceFirestore);
   instance.registerVersion(name, version, 'node');
 }
 
 registerFirestore(firebase);
 
-declare module '@firebase/app-types' {
-  interface FirebaseNamespace {
-    firestore?: {
-      (app?: FirebaseApp): types.FirebaseFirestore;
-      Blob: typeof types.Blob;
-      CollectionReference: typeof types.CollectionReference;
-      DocumentReference: typeof types.DocumentReference;
-      DocumentSnapshot: typeof types.DocumentSnapshot;
-      FieldPath: typeof types.FieldPath;
-      FieldValue: typeof types.FieldValue;
-      Firestore: typeof types.FirebaseFirestore;
-      GeoPoint: typeof types.GeoPoint;
-      Query: typeof types.Query;
-      QueryDocumentSnapshot: typeof types.QueryDocumentSnapshot;
-      QuerySnapshot: typeof types.QuerySnapshot;
-      Timestamp: typeof types.Timestamp;
-      Transaction: typeof types.Transaction;
-      WriteBatch: typeof types.WriteBatch;
-      setLogLevel: typeof types.setLogLevel;
-    };
-  }
-  interface FirebaseApp {
-    firestore?(): types.FirebaseFirestore;
-  }
-}

packages/firestore/index.ts

@@ -17,44 +17,19 @@
 
 import firebase from '@firebase/app';
 import { configureForFirebase } from './src/platform/config';
-import { registerFirestorePersistence } from './src/platform/persistence';
 import './src/platform_browser/browser_init';
 
-import * as types from '@firebase/firestore-types';
 import { FirebaseNamespace } from '@firebase/app-types';
 
 import { name, version } from './package.json';
+import {
+  PersistenceFirestore,
+  PublicPersistenceFirestore
+} from "./src/api/persistence";
 
 export function registerFirestore(instance: FirebaseNamespace): void {
-  configureForFirebase(instance);
-  registerFirestorePersistence(instance);
+  configureForFirebase(instance, PublicPersistenceFirestore, PersistenceFirestore);
   instance.registerVersion(name, version);
 }
 
 registerFirestore(firebase);
-
-declare module '@firebase/app-types' {
-  interface FirebaseNamespace {
-    firestore?: {
-      (app?: FirebaseApp): types.FirebaseFirestore;
-      Blob: typeof types.Blob;
-      CollectionReference: typeof types.CollectionReference;
-      DocumentReference: typeof types.DocumentReference;
-      DocumentSnapshot: typeof types.DocumentSnapshot;
-      FieldPath: typeof types.FieldPath;
-      FieldValue: typeof types.FieldValue;
-      Firestore: typeof types.FirebaseFirestore;
-      GeoPoint: typeof types.GeoPoint;
-      Query: typeof types.Query;
-      QueryDocumentSnapshot: typeof types.QueryDocumentSnapshot;
-      QuerySnapshot: typeof types.QuerySnapshot;
-      Timestamp: typeof types.Timestamp;
-      Transaction: typeof types.Transaction;
-      WriteBatch: typeof types.WriteBatch;
-      setLogLevel: typeof types.setLogLevel;
-    };
-  }
-  interface FirebaseApp {
-    firestore?(): types.FirebaseFirestore;
-  }
-}

packages/firestore/register-module.ts

@@ -0,0 +1,44 @@
+/**
+ * @license
+ * Copyright 2020 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import * as types from '@firebase/firestore-types';
+
+declare module '@firebase/app-types' {
+  interface FirebaseNamespace {
+    firestore?: {
+      (app?: FirebaseApp): types.FirebaseFirestore;
+      Blob: typeof types.Blob;
+      CollectionReference: typeof types.CollectionReference;
+      DocumentReference: typeof types.DocumentReference;
+      DocumentSnapshot: typeof types.DocumentSnapshot;
+      FieldPath: typeof types.FieldPath;
+      FieldValue: typeof types.FieldValue;
+      Firestore: typeof types.FirebaseFirestore;
+      GeoPoint: typeof types.GeoPoint;
+      Query: typeof types.Query;
+      QueryDocumentSnapshot: typeof types.QueryDocumentSnapshot;
+      QuerySnapshot: typeof types.QuerySnapshot;
+      Timestamp: typeof types.Timestamp;
+      Transaction: typeof types.Transaction;
+      WriteBatch: typeof types.WriteBatch;
+      setLogLevel: typeof types.setLogLevel;
+    };
+  }
+  interface FirebaseApp {
+    firestore?(): types.FirebaseFirestore;
+  }
+}

packages/firestore/rollup.config.js

@@ -200,17 +200,6 @@ const es2017Builds = [
     },
     plugins: es2017BuildPlugins,
     external: id => deps.some(dep => id === dep || id.startsWith(`${dep}/`))
-<<<<<<< HEAD
-  },
-  {
-    input: 'index.ts',
-    output: {
-      file: pkg.esm2017Minified,
-      format: 'es',
-      sourcemap: true
-    },
-    plugins: es2017MinifiedBuildPlugins,
-    external: id => deps.some(dep => id === dep || id.startsWith(`${dep}/`))
   },
   {
     input: 'index.memory.ts',
@@ -227,8 +216,6 @@ const es2017Builds = [
       }
       return deps.some(dep => id === dep || id.startsWith(`${dep}/`));
     }
-=======
->>>>>>> master
   }
 ];
 

packages/firestore/src/api/database.ts

@@ -21,11 +21,7 @@ import { FirebaseApp } from '@firebase/app-types';
 import { FirebaseService, _FirebaseApp } from '@firebase/app-types/private';
 import { DatabaseId, DatabaseInfo } from '../core/database_info';
 import { ListenOptions } from '../core/event_manager';
-import {
-  FirestoreClient,
-  PersistenceFactory,
-  newMemoryPersistence
-} from '../core/firestore_client';
+import { FirestoreClient } from '../core/firestore_client';
 import {
   Bound,
   Direction,
@@ -104,6 +100,8 @@ import {
 } from './user_data_converter';
 import { FirebaseAuthInternalName } from '@firebase/auth-interop-types';
 import { Provider } from '@firebase/component';
+import { newMemoryPersistence } from '../local/memory_persistence';
+import { PersistenceFactory } from '../local/persistence';
 
 // settings() defaults:
 const DEFAULT_HOST = 'firestore.googleapis.com';
@@ -142,7 +140,7 @@ export interface FirestoreDatabase {
  * user-supplied firestore.Settings object. This is a separate type so that
  * defaults can be supplied and the value can be checked for equality.
  */
-export class FirestoreSettings {
+class FirestoreSettings {
   /** The hostname to connect to. */
   readonly host: string;
 
@@ -292,15 +290,15 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
   private readonly _persistenceKey: string;
   private _credentials: CredentialsProvider;
   private readonly _firebaseApp: FirebaseApp | null = null;
-  private _settings: FirestoreSettings;
+  protected _settings: FirestoreSettings;
 
   // The firestore client instance. This will be available as soon as
   // configureClient is called, but any calls against it will block until
   // setup has completed.
   //
   // Operations on the _firestoreClient don't block on _firestoreReady. Those
   // are already set to synchronize on the async queue.
-  private _firestoreClient: FirestoreClient | undefined;
+  protected _firestoreClient: FirestoreClient | undefined;
 
   // Public for use in tests.
   // TODO(mikelehen): Use modularized initialization instead.
@@ -443,12 +441,12 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
     if (!this._firestoreClient) {
       // Kick off starting the client but don't actually wait for it.
       // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      this._configureClient(newMemoryPersistence);
+      this._configureClient(newMemoryPersistence, /* persistenceSettings= */ {});
     }
     return this._firestoreClient as FirestoreClient;
   }
 
-  private makeDatabaseInfo(): DatabaseInfo {
+  protected makeDatabaseInfo(): DatabaseInfo {
     return new DatabaseInfo(
       this._databaseId,
       this._persistenceKey,
@@ -458,8 +456,9 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
     );
   }
 
-  private _configureClient(
-    persistenceFactory: PersistenceFactory
+  protected _configureClient<T>(
+    persistenceFactory: PersistenceFactory<T>,
+    persistenceSettings: T
   ): Promise<void> {
     assert(!!this._settings.host, 'FirestoreSettings.host is not set');
 
@@ -474,7 +473,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
       this._queue
     );
 
-    return this._firestoreClient.start(persistenceFactory);
+    return this._firestoreClient.start(persistenceFactory, persistenceSettings);
   }
 
   private createDataConverter(databaseId: DatabaseId): UserDataConverter {
@@ -2622,7 +2621,7 @@ function applyFirestoreDataConverter<T>(
 
 // We're treating the variables as class names, so disable checking for lower
 // case variable names.
-export const PublicFirestore = makeConstructorPrivate(
+export const PublicMemoryFirestore = makeConstructorPrivate(
   Firestore,
   'Use firebase.firestore() instead.'
 );