[Multi-Tab] Adding basic functionality for TabNotificationChannel #483

schmidt-sebastian · 2018-02-02T18:22:14Z

This PR adds the InstanceState manipulation to the TabNotificationChannel. This is the basic functionality needed for secondary tabs as per go/multi-tab.

Still missing are:

Notifications for newly added instances.
Persisting of mutation batch and query target state (as performed by the primary)

googlebot · 2018-02-02T18:22:31Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

mikelehen

Despite the amount of feedback, I think this actually looks pretty good. :-)

mikelehen · 2018-02-03T18:05:26Z

packages/firestore/src/core/types.ts

+/**
+ * A randomly-generated key assigned to each Firestore instance at startup.
+ */
+export type InstanceKey = string;


Is this going to be used outside of tab_notification_channel? If not, I'm not sure it needs to be in types.ts...

It's part of the public interface in this class and will eventually be used to create values of this type. I don't think it will ever be used without pulling in the multi-tab code, so I I'm ok with moving it there.

mikelehen · 2018-02-03T18:07:13Z

packages/firestore/src/local/tab_notification_channel.ts

+ * changes. Primary tabs will further update the state of each mutation batch
+ * and query target.
+ */
+export interface TabNotificationChannel {


This seems to be a storage-agnostic interface, but the comment is very WebStorage-specific. Can you rework the comment?

Updated the comment.

mikelehen · 2018-02-03T18:12:39Z

packages/firestore/src/local/tab_notification_channel.ts

+}
+
+// Visible for testing
+export interface InstanceStateSchema {


comments? (something about this being the JSONified format we use in LocalStorage and the InstanceState class serializes to/from this)

Comment added.

mikelehen · 2018-02-03T18:14:32Z

packages/firestore/src/local/tab_notification_channel.ts

+// Visible for testing
+export interface InstanceStateSchema {
+  instanceKey?: string;
+  updateTime: number;


lastUpdateTime?

mikelehen · 2018-02-03T18:14:55Z

packages/firestore/src/local/tab_notification_channel.ts

+export interface InstanceStateSchema {
+  instanceKey?: string;
+  updateTime: number;
+  activeTargets: number[];


activeTargetIds ?

mikelehen · 2018-02-03T23:15:43Z

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

+export async function testLocalStorageNotificationChannel(
+  ownerId: string,
+  existingMutationBatches: BatchId[],
+  existingQueryTargets: TargetId[]


BatchIds, TargetIds

Updated, but I don't think this is more of a nice-to-have since this is typed to begin with.

Yeah. But since we also have actual MutationBatch and Target classes in the client, it's confusing to mismatch the names and types (for humans, that is... the compiler can keep it straight :-))

mikelehen · 2018-02-03T23:18:25Z

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

+  existingMutationBatches: BatchId[],
+  existingQueryTargets: TargetId[]
+): Promise<TabNotificationChannel> {
+  window.localStorage.clear();


I wonder if this is dangerous to do... You could imagine the test runner using local storage or something... Consider iterating localStorage and only deleting things with the firestore prefix... just in case?

Hm. Hrmm. Okay.

mikelehen · 2018-02-03T23:19:30Z

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

+  window.localStorage.clear();
+  const instances = [];
+
+  if (existingMutationBatches.length + existingQueryTargets.length != 0) {


Would existingMutatitonBatches.length > 0 || existingQueryTargets.length > 0 be a more natural way to express this?

(for a brief moment I actually thought this was checking if the two arrays cancelled each other out or something... but that's silly since length can't be negative)

Certainly :) Updated.

mikelehen · 2018-02-03T23:23:16Z

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

+  const instances = [];
+
+  if (existingMutationBatches.length + existingQueryTargets.length != 0) {
+    const secondaryChannel = new LocalStorageNotificationChannel(


This feels a bit dirty, I'd add a comment.

// HACK: Create a secondary channel to seed the data into LocalStorage.
// NOTE: We don't call shutdown() on it because that would delete the data.

mikelehen · 2018-02-03T23:24:03Z

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

+  existingQueryTargets: TargetId[]
+): Promise<TabNotificationChannel> {
+  window.localStorage.clear();
+  const instances = [];


knownInstances ?

mikelehen

Despite the amount of feedback, I think this is looking quite good. :-)

mikelehen · 2018-02-07T01:48:46Z

packages/firestore/src/local/instance_metadata_channel.ts

+  private started = false;
+
+  constructor(private persistenceKey: string, private instanceKey: string) {
+    this.localStorage = window ? window.localStorage : undefined;


If you run this in node you'll get ReferenceError: window is not defined. You need to do typeof window !== 'undefined' to check for the existence of globals.

Rather than accept localStorage being undefined, I think we should put the isAvailable() assert in here [you're welcome to update IndexedDbPersistence for consistency :-)].

I think the fact that this compiles, even though localStorage doesn't have |undefined on its type means we aren't compiling with --strictNullChecks. :-( I've opened b/73018483 to track getting it enabled.

I like the "let's add lint checks as we see problems" approach you are taking. I moved the isAvailable check to the constructor to sidestep this issue.

mikelehen · 2018-02-07T01:49:36Z

packages/firestore/src/local/instance_metadata_channel.ts

+      this.instanceKey
+    );
+    this.instanceKeyRe = new RegExp(
+      `^fs_instances_${persistenceKey}_[^_]{20}$`


Shouldn't you be using the constants for 'fs' and 'instances'?

I also don't know why we should be enforcing that instance keys are 20 characters here. We don't guarantee or enforce that anywhere else, do we? AFAIK that's an implementation detail that this class shouldn't care about.

I thought it was pretty ugly to use both constants here, but now that is only one constant, it does seem like the better approach.

mikelehen · 2018-02-07T02:05:10Z

packages/firestore/src/local/instance_metadata_channel.ts

+  }
+
+  return key.split('_')[dataSegement];
+}


This key creation / parsing stuff is annoyingly asymmetric, and passing a dataSegment number is fragile / unreadable, especially since the "fs" prefix segment is added implicitly by toLocalStorageKey() but then you have to account for it in fromLocalStorageKey() in your dataSegment number.

I'd recommend creating toLocalStorageKey(instanceKey: string): string and fromLocalStorageKey(localStorageKey: string): string as methods on WebStorageMetadataChannel as inverses of each other. fromLocalStorageKey() can assert that it matches the regex as a sanity check.

Done (methods are called toLocalStorageClientKey and fromLocalStorageClientKey to allow for me to add new formats later).

mikelehen · 2018-02-07T02:09:37Z

packages/firestore/src/local/instance_metadata_channel.ts

+
+// The format of an instance key is:
+//   fs_instances_<persistence_prefix>_<instance_key>
+const INSTANCE_KEY_NAMESPACE = 'instances';


Since we don't have any other local storage data, I'd recommend just having const INSTANCE_KEY_PREFIX = 'fs_instances';

This seems like premature generalization.

Sounds good.

mikelehen · 2018-02-07T18:10:49Z

packages/firestore/src/local/instance_metadata_channel.ts

+
+/**
+ * The `InstanceMetadataChannel` keeps track of the global state of the
+ * mutations and query targets for all active instances of the current project.


Remove "of the current project" or change to "with the same persistence key (i.e. project ID and FirebaseApp name)" ?

mikelehen · 2018-02-07T19:45:29Z

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

+      for (const targetId of expectedTargets) {
+        missingTargets = missingTargets.delete(targetId);
+      }
+      expect(missingTargets.size).to.equal(0);


If SortedSet had a .toArray() I think that would clean this up too, right? You could just expect(actualTargets.toArray()).to.deep.equal(expectedTargets.slice().sort())?

Done. Ended up using to.have.members here.

mikelehen · 2018-02-07T19:45:52Z

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

+
+      expect(
+        notificationChannel.getMinimumGloballyPendingMutation()
+      ).to.be.equal(minBatchId);


more unnecessary bes.

mikelehen · 2018-02-07T19:48:29Z

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

+      notificationChannel.addLocallyActiveQueryTarget(4);
+      verifyState(1, [3, 4]);
+
+      notificationChannel.addLocalPendingMutation(0);


this is technically invalid usage too... It should be >= 3

Added comment

mikelehen · 2018-02-07T19:50:24Z

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

+      ).to.be.equal(minBatchId);
+    };
+
+    it('with existing instance', () => {


should this be 'from local instance'?

Renamed to 'with data from existing client'. It's actually what I refer to as a remote instance.

mikelehen · 2018-02-07T19:55:50Z

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

+        activeTargetIds: [5, 6],
+        minMutationBatchId: 0,
+        maxMutationBatchId: 0
+      };


I think I'd be much happier to see us actually creating multiple channels and testing that modifications via one are surfaced correctly in others. As-is, we're skipping a big chunk of the implementation (LocalStorage) and making our tests dependent on implementation details (the LocalStorage schema) unnecessarily.

That would also mean that if we implemented persistence on Node one day and used a communication channel other than LocalStorage, this testing would cover it, so it's more valuable.

Some of this testing exists hidden in testWebStorageSharedClientState. I updated the test here to rely on the schema embedded in RemoteClientState, but as pointed out before, I can't actually test the notifications.

[AUTOMATED]: Prettier Code Styling

schmidt-sebastian · 2018-02-08T01:13:32Z

This PR is ready for review again.

mikelehen

A few last nits, but looks good to me!

mikelehen · 2018-02-08T18:21:58Z

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

+      window.addEventListener = (type, callback) => {
+        expect(type).to.be.equal('storage');
+        storageCallback = callback;
+      };


Ohhh... geez. That's insidious. That means we're not going to be able to test any multi-tab stuff with integration tests, right? That's scary... I think it's probably worth putting some thought into where we want to end up from a testing perspective.

Ideas:

Create an interface over LocalStorage so we can create a FakeLocalStorage implementation that can be used in tests (injected via modularized initialization or whatever). We could use that in these tests and remove storageCallback.

Create a TestSharedClientState implementation of SharedClientState that we substitute in tests (I think this is what you did in your prototype PR with spec tests). This is probably okay, though I generally like to substitute mocks/fakes at the lowest layer possible so we test as much implementation code as possible.

Apparently you can manufacture local storage events in the local tab (https://stackoverflow.com/a/25514935). Maybe we could leverage that somehow (e.g. have a test-only mode on WebStorageSharedClientState that tells it to auto-raise an event after any LocalStorage write it does). Kinda' hacky, and we'd then need to tolerate events from our own instance row, but it might work...

As I mentioned on HipChat, I'm also interested in our spec testing approach. It might be really interesting to allow tests like:

.withClient(1) .userSets('collection/key', { foo: 'bar' }) .expectEvents( ... ) .withClient(2) .expectEvents( ... )

I am fine with this PR using this storageCallback mechanism for now, but I think we need to figure out where we want to land testing-wise, and if we end up creating a FakeLocalStorage type thing, we should rework these tests to use it.

mikelehen · 2018-02-08T19:21:36Z

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

+  ) {
+    // HACK: Create a secondary client state to seed data into LocalStorage.
+    // NOTE: We don't call shutdown() on it because that would delete the data.
+    const secondaryChannel = new WebStorageSharedClientState(


secondaryClientState

mikelehen · 2018-02-08T19:28:27Z

packages/firestore/src/local/shared_client_state.ts

+  constructor(
+    readonly clientKey: ClientKey,
+    public lastUpdateTime: Date,
+    public activeTargetIds: SortedSet<TargetId>,


lastUpdateTime and activeTargetIds can be readonly too, I think?

Yes, fixed.

mikelehen · 2018-02-08T19:28:40Z

packages/firestore/src/local/shared_client_state.ts

+ * of its pending mutation batch IDs.
+ */
+class RemoteClientState implements ClientState {
+  constructor(


Can the constructor be private?

mikelehen · 2018-02-08T19:29:40Z

packages/firestore/src/local/shared_client_state.ts

+  ) {}
+
+  /**
+   * Parses a ClientState from the JSON representation in LocalStorage.


ClientState => RemoteClientState ?

mikelehen · 2018-02-08T19:39:02Z

packages/firestore/src/local/shared_client_state.ts

+    this.started = false;
+  }
+
+  private handleStorageEvent(event: StorageEvent): void {


Just FYI- It occurred to me that once you start triggering events / callbacks to SyncEngine on changes we're going to need to make sure we are on the AsyncQueue, so you'll probably need to make WebStorageSharedClientState have an AsyncQueue reference, etc.

Since JS is single-threaded and we're not calling back into any other client code here, it doesn't matter for now.

mikelehen · 2018-02-08T19:45:59Z

packages/firestore/src/local/shared_client_state.ts

+  private readonly clientStateKeyRe: RegExp;
+  private started = false;
+
+  constructor(private persistenceKey: string, private clientKey: string) {


clientKey => localClientKey (to avoid confusion with the generic 'clientKey' variables you use in this code)?

Done. This also works nicely with the method localClientState().

Also made this readonly.

mikelehen · 2018-02-08T19:46:40Z

packages/firestore/src/local/shared_client_state.ts

+      );
+    }
+    this.storage = window.localStorage;
+    this.storageKey = this.toLocalStorageClientKey(this.clientKey);


storageKey => localClientStorageKey ? (or remove this and just generate it on-demand from clientKey).

Gil suggested this variable in the first PR, which I think makes sense. Renamed.

mikelehen · 2018-02-08T19:51:00Z

packages/firestore/src/local/shared_client_state.ts

+      );
+      const clientKey = this.fromLocalStorageClientKey(event.key);
+      if (clientKey) {
+        if (event.newValue == null) {


Consider adding logging for these LocalStorage notifications (can do in a later PR, e.g. when adding callbacks to SyncEngine if you prefer).

I think we would want to log the actual notifications that we deliver to SyncEngine, so I am going to defer on this until we get to that part.

schmidt-sebastian

Feedback addressed

schmidt-sebastian · 2018-02-08T20:09:02Z

packages/firestore/src/local/shared_client_state.ts

+ * of its pending mutation batch IDs.
+ */
+class RemoteClientState implements ClientState {
+  constructor(


schmidt-sebastian · 2018-02-08T20:09:09Z

packages/firestore/src/local/shared_client_state.ts

+  constructor(
+    readonly clientKey: ClientKey,
+    public lastUpdateTime: Date,
+    public activeTargetIds: SortedSet<TargetId>,


Yes, fixed.

schmidt-sebastian · 2018-02-08T20:09:21Z

packages/firestore/src/local/shared_client_state.ts

+  ) {}
+
+  /**
+   * Parses a ClientState from the JSON representation in LocalStorage.


schmidt-sebastian · 2018-02-08T20:10:36Z

packages/firestore/src/local/shared_client_state.ts

+  private readonly clientStateKeyRe: RegExp;
+  private started = false;
+
+  constructor(private persistenceKey: string, private clientKey: string) {


Done. This also works nicely with the method localClientState().

Also made this readonly.

schmidt-sebastian · 2018-02-08T20:12:05Z

packages/firestore/src/local/shared_client_state.ts

+      );
+    }
+    this.storage = window.localStorage;
+    this.storageKey = this.toLocalStorageClientKey(this.clientKey);


Gil suggested this variable in the first PR, which I think makes sense. Renamed.

schmidt-sebastian · 2018-02-08T20:13:35Z

packages/firestore/src/local/shared_client_state.ts

+      );
+      const clientKey = this.fromLocalStorageClientKey(event.key);
+      if (clientKey) {
+        if (event.newValue == null) {


I think we would want to log the actual notifications that we deliver to SyncEngine, so I am going to defer on this until we get to that part.

schmidt-sebastian · 2018-02-08T20:14:00Z

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

+  ) {
+    // HACK: Create a secondary client state to seed data into LocalStorage.
+    // NOTE: We don't call shutdown() on it because that would delete the data.
+    const secondaryChannel = new WebStorageSharedClientState(


schmidt-sebastian requested review from gauntface, jshcrowthe, mikelehen and wilhuff as code owners February 2, 2018 18:22

google-oss-bot added the needs-triage label Feb 2, 2018

schmidt-sebastian force-pushed the multitab-instancestate branch from 280374e to 1530c9c Compare February 2, 2018 18:27

schmidt-sebastian removed request for gauntface, mikelehen, jshcrowthe and wilhuff February 2, 2018 18:30

schmidt-sebastian force-pushed the multitab-instancestate branch from 4e54f22 to ca28823 Compare February 2, 2018 18:52

schmidt-sebastian added 2 commits February 2, 2018 11:13

Adding basic functionality for TabNotificationChannel

8dbcb8e

[AUTOMATED]: Prettier Code Styling

c5099ff

schmidt-sebastian force-pushed the multitab-instancestate branch from ca28823 to c5099ff Compare February 2, 2018 19:13

Comment cleanup

4c3fa4c

schmidt-sebastian changed the title ~~Multitab instancestate~~ Adding basic functionality for TabNotificationChannel Feb 2, 2018

schmidt-sebastian requested a review from mikelehen February 2, 2018 19:25

schmidt-sebastian assigned mikelehen Feb 2, 2018

Test fix

1c2fa18

schmidt-sebastian force-pushed the multitab-instancestate branch from d929370 to 1c2fa18 Compare February 2, 2018 19:54

mikelehen suggested changes Feb 3, 2018

View reviewed changes

Addressing comments

7cc1832

schmidt-sebastian force-pushed the multitab-instancestate branch from 057066e to 7cc1832 Compare February 6, 2018 01:25

schmidt-sebastian requested a review from mikelehen February 6, 2018 01:26

Fixing lint errors

5532ac8

schmidt-sebastian changed the title ~~Adding basic functionality for TabNotificationChannel~~ [Multi-Tab] Adding basic functionality for TabNotificationChannel Feb 6, 2018

mikelehen suggested changes Feb 7, 2018

View reviewed changes

schmidt-sebastian added 2 commits February 7, 2018 12:27

Changing Instance to Client

a0f4545

Main review cleanup

ede438e

[AUTOMATED]: Prettier Code Styling

Merge branch 'master' into multitab-instancestate

0b42fc1

schmidt-sebastian force-pushed the multitab-instancestate branch from 88d79bb to a887823 Compare February 8, 2018 00:46

Fixing imports

522917e

schmidt-sebastian force-pushed the multitab-instancestate branch from a887823 to 522917e Compare February 8, 2018 00:52

schmidt-sebastian added 3 commits February 7, 2018 17:05

Cleaning up assertClientState

5542cd6

[AUTOMATED]: Prettier Code Styling

c384115

Renaming test file

ccf9a6f

mikelehen approved these changes Feb 8, 2018

View reviewed changes

Last round of feedback

2369f55

schmidt-sebastian commented Feb 8, 2018

View reviewed changes

schmidt-sebastian added 2 commits February 8, 2018 12:15

[AUTOMATED]: Prettier Code Styling

41252d4

Lint errors

e09fa4a

schmidt-sebastian merged commit 4159289 into firestore-multi-tab Feb 8, 2018

schmidt-sebastian deleted the multitab-instancestate branch March 26, 2018 21:33

firebase locked and limited conversation to collaborators Oct 23, 2019

[Multi-Tab] Adding basic functionality for TabNotificationChannel #483

[Multi-Tab] Adding basic functionality for TabNotificationChannel #483

Conversation

schmidt-sebastian commented Feb 2, 2018 • edited Loading

googlebot commented Feb 2, 2018

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Feb 8, 2018

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Feb 2, 2018 •

edited

Loading