Review

Author: schmidt-sebastian

packages/firestore/src/api/database.ts

@@ -96,7 +96,7 @@ import {
 import { UserDataWriter } from './user_data_writer';
 import { FirebaseAuthInternalName } from '@firebase/auth-interop-types';
 import { Provider } from '@firebase/component';
-import {Dict} from "../util/obj";
+import { Dict } from '../util/obj';
 
 // settings() defaults:
 const DEFAULT_HOST = 'firestore.googleapis.com';
@@ -339,7 +339,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
     validateExactNumberOfArgs('Firestore.settings', arguments, 1);
     validateArgType('Firestore.settings', 'object', 1, settingsLiteral);
 
-    if (contains(settingsLiteral as objUtils.Dict<{}>, 'persistence')) {
+    if (contains(settingsLiteral, 'persistence')) {
       throw new FirestoreError(
         Code.INVALID_ARGUMENT,
         '"persistence" is now specified with a separate call to ' +
@@ -552,15 +552,14 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
   }
 
   private static databaseIdFromApp(app: FirebaseApp): DatabaseId {
-    const options = app.options as Dict<unknown>;
-    if (contains(options, 'projectId')) {
+    if (!contains(app.options, 'projectId')) {
       throw new FirestoreError(
         Code.INVALID_ARGUMENT,
         '"projectId" not provided in firebase.initializeApp.'
       );
     }
 
-    const projectId = options['projectId'];
+    const projectId = app.options.projectId;
     if (!projectId || typeof projectId !== 'string') {
       throw new FirestoreError(
         Code.INVALID_ARGUMENT,
@@ -2622,7 +2621,7 @@ function applyFirestoreDataConverter<T>(
   return [convertedValue, functionName];
 }
 
-function contains(obj: Dict<unknown>, key: string): boolean {
+function contains(obj: object, key: string): obj is { key : unknown } {
   return Object.prototype.hasOwnProperty.call(obj, key);
 }
 

packages/firestore/src/core/sync_engine.ts

@@ -413,7 +413,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       const changes = await this.localStore.applyRemoteEvent(remoteEvent);
       // Update `receivedDocument` as appropriate for any limbo targets.
       remoteEvent.targetChanges.forEach((targetChange, targetId) => {
-        const limboResolution = this.activeLimboResolutionsByTarget.get(targetId);
+        const limboResolution = this.activeLimboResolutionsByTarget.get(
+          targetId
+        );
         if (limboResolution) {
           // Since this is a limbo resolution lookup, it's for a single document
           // and it could be added, modified, or removed, but not a combination.
@@ -491,7 +493,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
 
     // PORTING NOTE: Multi-tab only.
     this.sharedClientState.updateQueryState(targetId, 'rejected', err);
-    
+
     const limboResolution = this.activeLimboResolutionsByTarget.get(targetId);
     const limboKey = limboResolution && limboResolution.key;
     if (limboKey) {
@@ -755,7 +757,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
 
     this.remoteStore.unlisten(limboTargetId);
     this.activeLimboTargetsByKey = this.activeLimboTargetsByKey.remove(key);
-this.activeLimboResolutionsByTarget.delete(limboTargetId);
+    this.activeLimboResolutionsByTarget.delete(limboTargetId);
     this.pumpEnqueuedLimboResolutions();
   }
 
@@ -807,9 +809,10 @@ this.activeLimboResolutionsByTarget.delete(limboTargetId);
     ) {
       const key = this.enqueuedLimboResolutions.shift()!;
       const limboTargetId = this.limboTargetIdGenerator.next();
-      this.activeLimboResolutionsByTarget.set(limboTargetId, new LimboResolution(
-        key
-      ));
+      this.activeLimboResolutionsByTarget.set(
+        limboTargetId,
+        new LimboResolution(key)
+      );
       this.activeLimboTargetsByKey = this.activeLimboTargetsByKey.insert(
         key,
         limboTargetId

packages/firestore/src/model/values.ts

@@ -19,7 +19,7 @@ import * as api from '../protos/firestore_proto_api';
 
 import { TypeOrder } from './field_value';
 import { assert, fail } from '../util/assert';
-import { forEach, size } from '../util/obj';
+import { forEach, objectSize } from '../util/obj';
 import { ByteString } from '../util/byte_string';
 import { isNegativeZero } from '../util/types';
 import { DocumentKey } from './document_key';
@@ -162,7 +162,7 @@ function objectEquals(left: api.Value, right: api.Value): boolean {
   const leftMap = left.mapValue!.fields || {};
   const rightMap = right.mapValue!.fields || {};
 
-  if (size(leftMap) !== size(rightMap)) {
+  if (objectSize(leftMap) !== objectSize(rightMap)) {
     return false;
   }
 

packages/firestore/src/remote/watch_change.ts

@@ -595,11 +595,12 @@ export class WatchChangeAggregator {
   }
 
   private ensureTargetState(targetId: TargetId): TargetState {
-    if (!this.targetStates.has(targetId)) {
-      this.targetStates.set(targetId, new TargetState());
+    let result = this.targetStates.get(targetId);
+    if (!result) {
+      result = new TargetState();
+      this.targetStates.set(targetId, result);
     }
-
-    return this.targetStates.get(targetId)!;
+    return result;
   }
 
   private ensureDocumentTargetMapping(key: DocumentKey): SortedSet<TargetId> {

packages/firestore/src/util/obj.ts

@@ -21,7 +21,7 @@ export interface Dict<V> {
   [stringKey: string]: V;
 }
 
-export function size<V>(obj: object): number {
+export function objectSize<V>(obj: object): number {
   let count = 0;
   for (const key in obj) {
     if (Object.prototype.hasOwnProperty.call(obj, key)) {

packages/firestore/test/unit/local/web_storage_shared_client_state.test.ts

@@ -41,7 +41,7 @@ import { PlatformSupport } from '../../../src/platform/platform';
 import { AsyncQueue } from '../../../src/util/async_queue';
 import { FirestoreError } from '../../../src/util/error';
 import { AutoId } from '../../../src/util/misc';
-import { size } from '../../../src/util/obj';
+import { objectSize } from '../../../src/util/obj';
 import { SortedSet } from '../../../src/util/sorted_set';
 import {
   clearWebStorage,
@@ -103,7 +103,7 @@ class TestSharedClientSyncer implements SharedClientStateSyncer {
 
   get sharedClientState(): TestSharedClientState {
     return {
-      mutationCount: size(this.mutationState),
+      mutationCount: objectSize(this.mutationState),
       mutationState: this.mutationState,
       targetIds: this.activeTargets,
       targetState: this.queryState,

packages/firestore/test/unit/specs/spec_builder.ts

@@ -63,8 +63,13 @@ export interface LimboMap {
   [key: string]: TargetId;
 }
 
+export interface ActiveTargetSpec {
+  queries: SpecQuery[];
+  resumeToken: string;
+}
+
 export interface ActiveTargetMap {
-  [targetId: string]: { queries: SpecQuery[]; resumeToken: string };
+  [targetId: string]: ActiveTargetSpec;
 }
 
 /**

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -86,7 +86,7 @@ import { assert, fail } from '../../../src/util/assert';
 import { AsyncQueue, TimerId } from '../../../src/util/async_queue';
 import { FirestoreError } from '../../../src/util/error';
 import { primitiveComparator } from '../../../src/util/misc';
-import { forEach, size } from '../../../src/util/obj';
+import { forEach, objectSize } from '../../../src/util/obj';
 import { ObjectMap } from '../../../src/util/obj_map';
 import { Deferred, sequence } from '../../../src/util/promise';
 import {
@@ -116,6 +116,7 @@ import {
 import { MULTI_CLIENT_TAG } from './describe_spec';
 import { ByteString } from '../../../src/util/byte_string';
 import { SortedSet } from '../../../src/util/sorted_set';
+import { ActiveTargetMap, ActiveTargetSpec } from './spec_builder';
 
 const ARBITRARY_SEQUENCE_NUMBER = 2;
 
@@ -412,10 +413,7 @@ abstract class TestRunner {
 
   private expectedActiveLimboDocs: DocumentKey[];
   private expectedEnqueuedLimboDocs: DocumentKey[];
-  private expectedActiveTargets:  Map<
-    TargetId,
-    { queries: SpecQuery[]; resumeToken: string }
-    >;
+  private expectedActiveTargets: Map<TargetId, ActiveTargetSpec>;
 
   private networkEnabled = true;
 
@@ -465,10 +463,7 @@ abstract class TestRunner {
     this.maxConcurrentLimboResolutions = config.maxConcurrentLimboResolutions;
     this.expectedActiveLimboDocs = [];
     this.expectedEnqueuedLimboDocs = [];
-    this.expectedActiveTargets =new Map<
-      TargetId,
-      { queries: SpecQuery[]; resumeToken: string }
-      >();
+    this.expectedActiveTargets = new Map<TargetId, ActiveTargetSpec>();
     this.acknowledgedDocs = [];
     this.rejectedDocs = [];
     this.snapshotsInSyncListeners = [];
@@ -1215,7 +1210,7 @@ abstract class TestRunner {
       );
       delete actualTargets[targetId];
     });
-    expect(size(actualTargets)).to.equal(
+    expect(objectSize(actualTargets)).to.equal(
       0,
       'Unexpected active targets: ' + JSON.stringify(actualTargets)
     );
@@ -1713,9 +1708,7 @@ export interface StateExpectation {
   /**
    * Current expected active targets. Verified in each step until overwritten.
    */
-  activeTargets?: {
-    [targetId: number]: { queries: SpecQuery[]; resumeToken: string };
-  };
+  activeTargets?: ActiveTargetMap;
   /**
    * Expected set of callbacks for previously written docs.
    */

packages/firestore/test/util/helpers.ts

@@ -512,7 +512,7 @@ export function byteStringFromString(value: string): ByteString {
   return ByteString.fromBase64String(base64);
 }
 
-/** 
+/**
  * Decodes a base 64 decoded string.
  *
  * Note that this is typed to accept Uint8Arrays to match the types used