Next round of review comments

Author: schmidt-sebastian

packages/firestore/src/local/shared_client_state.ts

@@ -36,6 +36,9 @@ const CLIENT_STATE_KEY_PREFIX = 'fs_clients';
 // The format of the LocalStorage key that stores the mutation state is:
 //     fs_mutations_<persistence_prefix>_<batch_id> (for unauthenticated users)
 // or: fs_mutations_<persistence_prefix>_<batch_id>_<user_uid>
+//
+// 'user_uid' is last to avoid needing to escape '_' characters that it might
+// contain.
 const MUTATION_BATCH_KEY_PREFIX = 'fs_mutations';
 
 /**
@@ -53,11 +56,11 @@ export type ClientKey = string;
  * `SharedClientState` is primarily used for synchronization in Multi-Tab
  * environments. Each tab is responsible for registering its active query
  * targets and mutations. `SharedClientState` will then notify the listener
- * passed to `subscribe()` for updates to mutations and queries that originated
- * in other clients.
+ * assigned to `.syncEngine` for updates to mutations and queries that
+ * originated in other clients.
  *
- * To receive notifications, both `subscribe()` and `start()` have to be called
- * in order.
+ * To receive notifications, `.syncEngine` has to be assigned before calling
+ * `start()`.
  *
  * TODO(multitab): Add callbacks to SyncEngine
  */
@@ -115,7 +118,7 @@ interface MutationMetadataSchema {
 }
 
 /**
- * Holds the state of a mutation batch, including its user ID, batch ID annd
+ * Holds the state of a mutation batch, including its user ID, batch ID and
  * whether the batch is 'pending', 'acknowledged' or 'rejected'.
  */
 // Visible for testing
@@ -181,15 +184,18 @@ export class MutationMetadata {
   }
 
   toLocalStorageJSON(): string {
-    const batchState: MutationMetadataSchema = {
+    const batchMetadata: MutationMetadataSchema = {
       state: this.state
     };
 
     if (this.error) {
-      batchState.error = { code: this.error.code, message: this.error.message };
+      batchMetadata.error = {
+        code: this.error.code,
+        message: this.error.message
+      };
     }
 
-    return JSON.stringify(batchState);
+    return JSON.stringify(batchMetadata);
   }
 }
 
@@ -366,13 +372,14 @@ export class LocalClientState implements ClientState {
  */
 // TODO(multitab): Rename all usages of LocalStorage to WebStorage to better differentiate from LocalClient.
 export class WebStorageSharedClientState implements SharedClientState {
+  syncEngine: SharedClientStateSyncer | null;
+
   private readonly storage: Storage;
   private readonly localClientStorageKey: string;
   private readonly activeClients: { [key: string]: ClientState } = {};
   private readonly storageListener = this.handleLocalStorageEvent.bind(this);
   private readonly clientStateKeyRe: RegExp;
   private readonly mutationBatchKeyRe: RegExp;
-  private syncEngine: SharedClientStateSyncer | null;
   private user: User;
   private started = false;
 
@@ -395,7 +402,7 @@ export class WebStorageSharedClientState implements SharedClientState {
       `^${CLIENT_STATE_KEY_PREFIX}_${persistenceKey}_([^_]*)$`
     );
     this.mutationBatchKeyRe = new RegExp(
-      `^${MUTATION_BATCH_KEY_PREFIX}_${persistenceKey}_(\\d*)(?:_(.*))$`
+      `^${MUTATION_BATCH_KEY_PREFIX}_${persistenceKey}_(\\d+)(?:_(.*))?$`
     );
   }
 
@@ -404,17 +411,9 @@ export class WebStorageSharedClientState implements SharedClientState {
     return typeof window !== 'undefined' && window.localStorage != null;
   }
 
-  subscribe(syncEngine: SharedClientStateSyncer) {
-    this.syncEngine = syncEngine;
-  }
-
   // TODO(multitab): Handle user changes.
   start(initialUser: User, knownClients: ClientKey[]): void {
     assert(!this.started, 'WebStorageSharedClientState already started');
-    assert(
-      this.syncEngine !== null,
-      'Start() called before subscribing to events'
-    );
     window.addEventListener('storage', this.storageListener);
 
     for (const clientKey of knownClients) {
@@ -486,6 +485,7 @@ export class WebStorageSharedClientState implements SharedClientState {
   }
 
   private handleLocalStorageEvent(event: StorageEvent): void {
+    console.log(event.key);
     if (!this.started) {
       return;
     }
@@ -497,21 +497,21 @@ export class WebStorageSharedClientState implements SharedClientState {
         'Received LocalStorage notification for local change.'
       );
 
-      if (event.newValue == null) {
-        if (this.clientStateKeyRe.test(event.key)) {
-          const clientId = this.fromLocalStorageClientStateKey(event.key);
-          delete this.activeClients[clientId];
-        }
-      } else {
-        if (this.clientStateKeyRe.test(event.key)) {
+      if (this.clientStateKeyRe.test(event.key)) {
+        if (event.newValue != null) {
           const clientState = this.fromLocalStorageClientState(
             event.key,
             event.newValue
           );
           if (clientState) {
             this.activeClients[clientState.clientId] = clientState;
           }
-        } else if (this.mutationBatchKeyRe.test(event.key)) {
+        } else {
+          const clientId = this.fromLocalStorageClientStateKey(event.key);
+          delete this.activeClients[clientId];
+        }
+      } else if (this.mutationBatchKeyRe.test(event.key)) {
+        if (event.newValue !== null) {
           const mutationMetadata = this.fromLocalStorageMutationMetadata(
             event.key,
             event.newValue
@@ -618,6 +618,11 @@ export class WebStorageSharedClientState implements SharedClientState {
   private async handleMutationBatchEvent(
     mutationBatch: MutationMetadata
   ): Promise<void> {
+    assert(
+      this.syncEngine !== null,
+      'syncEngine property must be set in order to handle events'
+    );
+
     if (mutationBatch.user.uid !== this.user.uid) {
       debug(
         LOG_TAG,

packages/firestore/src/local/shared_client_state_syncer.ts

@@ -18,7 +18,7 @@ import { BatchId } from '../core/types';
 import { FirestoreError } from '../util/error';
 
 /**
- * A interface that describes the actions the SharedClientState class needs to
+ * An interface that describes the actions the SharedClientState class needs to
  * perform on a cooperating synchronization engine.
  */
 export interface SharedClientStateSyncer {

packages/firestore/src/remote/remote_syncer.ts

@@ -21,7 +21,7 @@ import { FirestoreError } from '../util/error';
 import { RemoteEvent } from './remote_event';
 
 /**
- * A interface that describes the actions the RemoteStore needs to perform on
+ * An interface that describes the actions the RemoteStore needs to perform on
  * a cooperating synchronization engine.
  */
 export interface RemoteSyncer {

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

@@ -118,7 +118,7 @@ export async function testWebStorageSharedClientState(
 
     knownInstances.push(SECONDARY_INSTANCE_KEY);
 
-    secondaryClientState.subscribe(new NoOpSharedClientStateSyncer());
+    secondaryClientState.syncEngine = new NoOpSharedClientStateSyncer();
     await secondaryClientState.start(user, [SECONDARY_INSTANCE_KEY]);
 
     for (const batchId of existingMutationBatchIds) {
@@ -136,7 +136,7 @@ export async function testWebStorageSharedClientState(
     TEST_PERSISTENCE_PREFIX,
     instanceKey
   );
-  sharedClientState.subscribe(sharedClientSyncer);
+  sharedClientState.syncEngine = sharedClientSyncer;
   await sharedClientState.start(user, knownInstances);
   return sharedClientState;
 }