Factor out Multi-Tab code #2882

schmidt-sebastian · 2020-04-08T00:27:47Z

This PR is based on the changes in #2828 (and the reason for #2828) and meant to reduce the size of the memory-only build.

It creates two new classes: MultiTabLocalStore and MultiTabSyncEngine that each inherit from their old structures. It then moves all fully-encapsulated Multi-Tab only code the new classes which are only used in the full Firestore build.

Since MultiTabLocalStore only works with IndexedDbPersistence, this change also allows us to remove a few methods from MemoryPersistence that were never used. I did this by removing the methods from the shared interfaces and moving the method comment down to the IndexedDbBimplementation.

As a third thing, it changes LocalStoreTester. LocalStoreTester needs to know the source of the QueryEngine (whether index-free or not), but that plumbing is pretty gnarly, so I opted for the pattern used in C++ and added a type: https://github.com/firebase/firebase-ios-sdk/blob/d9b7a33f0c0464e72e894aa7c207efc07dadbbbe/Firestore/core/test/firebase/firestore/local/counting_query_engine.h#L62

google-oss-bot · 2020-04-08T01:16:48Z

Binary Size Report

Affected SDKs

@firebase/firestore

Type Base (a36b51b) Head (751324f) Diff

browser 248 kB 248 kB +353 B (+0.1%)

esm2017 194 kB 194 kB +17 B (+0.0%)

main 488 kB 489 kB +752 B (+0.2%)

module 246 kB 246 kB +333 B (+0.1%)

`@firebase/firestore/memory`

Type	Base (`a36b51b`)	Head (`751324f`)	Diff
browser	195 kB	189 kB	-6.36 kB (-3.3%)
esm2017	152 kB	149 kB	-3.31 kB (-2.2%)
main	377 kB	366 kB	-11.5 kB (-3.1%)
module	194 kB	188 kB	-6.15 kB (-3.2%)

`firebase`

Type	Base (`a36b51b`)	Head (`751324f`)	Diff
firebase-firestore.js	290 kB	290 kB	+314 B (+0.1%)
firebase-firestore.memory.js	239 kB	232 kB	-6.11 kB (-2.6%)
firebase.js	823 kB	824 kB	+316 B (+0.0%)

Test Logs

Base (a36b51b4): https://github.com/firebase/firebase-js-sdk/actions/runs/83400898
Head (751324f0): https://github.com/firebase/firebase-js-sdk/actions/runs/83426467

schmidt-sebastian · 2020-04-09T22:22:20Z

Note: Some of the unit tests are failing after Master merge. I will fix them later tonight.

rafikhan

Below is my feedback for multi-tab.

Along the way I found a small issue in the Firestore class. The constructor takes a persistenceProvider parameter and I think you meant this to be named componentProvider.

rafikhan · 2020-04-17T23:17:35Z

packages/firestore/src/core/component_provider.ts

@@ -210,6 +197,68 @@ export class MemoryLruComponentProvider extends MemoryComponentProvider {
 * Provides all components needed for Firestore with IndexedDB persistence.
 */
 export class IndexedDbComponentProvider extends MemoryComponentProvider {


I'm unable to comment on lines of code outside of what's changed however I see a few high level things:

MemoryComponentProvider should explicitly implicitly implement ComponentProvider. Currently it doesn't.

I think these assertions provide the correct guarantee with minimal lines of code but I wonder if instead you modified the ComponentProvider interface to take generic arguments for every member with defaults and then overrode it in MemoryComponentProvider and again here it would get you compile time safety and let you eliminate these assertions. It would be a large list of generic constraints which comes with its own problems but might be worth thinking about.

Fixed (also renamed the constructor argument in FirestoreClient).

I opted not to go down the route and make this generic, but I did overwrite some of the members and use more explicit types. This gets rid of some of the asserts. Let me know if that is enough.

As part of the Master merge, I also had to fix the spec tests to be aware of the differences between the different persistence implementations.

rafikhan · 2020-04-18T00:02:17Z

packages/firestore/src/core/component_provider.ts

-import { LocalStore } from '../local/local_store';
-import { SyncEngine } from './sync_engine';
+import { LocalStore, MultiTabLocalStore } from '../local/local_store';
+import { MultiTabSyncEngine, SyncEngine } from './sync_engine';


This review doesn't include MultiTabSyncEngine but I have some thoughts on how to move SharedClientState and related properties into it. Most of these options will make the code ugly, awkward or might not be viable but want to throw it out there anyway.

In any method in SyncEngine that has multi-tab logic is it possible to refactor it to not be in the middle? For example removeAndCleanupTarget() only has one line at the top. Could you do that to everything else? If so, you could rely on method overloading in MultiTabSyncEngine to do pre/post processing and call the base class method in the appropriate place. If the logic can't be split it might be worth overloading the method entirely and relying on unit tests to ensure consistency.

Alternatively, in methods where you need a return value from SharedClientState you can modify the method to have an optional callback parameter that usually gives you a default value but in MultiTabSyncEngine you can overload the method and call into the base with an argument for the callback that gives you a real result. You can do the same for notification only methods OR raise an event that MultiTabSyncEngine can subscribe to. The latter is super weird but eliminates the reference entirely without needing a stub implementation.

I looked through all the calls to SharedClientState to see if we can improve the logic and remove some of the tight coupling. Unfortunately, I think in almost all cases the current logic is simpler than what we would have if we pulled out the code:

If we overwrite the methods to extract one line, it creates an interaction model that is much harder to follow. While it is relatively straightforward to pull out the first or last line in some of these methods, it essentially hides these lines from view if the use is not aware that they are overwritten in MultiTabSyncEngine.

In some cases (like removeAndCleanupTarget()) this is not actually all that bad - but it creates some inconsistencies between target allocation and target cleanup. I would rather keep all of this code in one place.

I did create an overwrite for applyOnlineStateChange. This method has different behavior for Multi-Tab clients and has no "mirrored" behavior in SyncEngine.

schmidt-sebastian

Thanks for the review! I applied some of your suggestions and replied to your comments when I decided not to. To truly refactor out all Mutli-Tab code, the code flow becomes even more convoluted than it is, so at the moment I prefer to only refactor methods where we don't have to drastically alter the code flow.

I did experiment with creating a stand alone MultiTabComponentProvider. This is much cleaner, but we unfortunately don't know whether the user would like to opt into Multi-Tab persistence until the client has been initialized and cannot register the specific provider at compile time.

schmidt-sebastian · 2020-04-20T17:48:01Z

packages/firestore/src/core/component_provider.ts

-import { LocalStore } from '../local/local_store';
-import { SyncEngine } from './sync_engine';
+import { LocalStore, MultiTabLocalStore } from '../local/local_store';
+import { MultiTabSyncEngine, SyncEngine } from './sync_engine';


I looked through all the calls to SharedClientState to see if we can improve the logic and remove some of the tight coupling. Unfortunately, I think in almost all cases the current logic is simpler than what we would have if we pulled out the code:

If we overwrite the methods to extract one line, it creates an interaction model that is much harder to follow. While it is relatively straightforward to pull out the first or last line in some of these methods, it essentially hides these lines from view if the use is not aware that they are overwritten in MultiTabSyncEngine.

In some cases (like removeAndCleanupTarget()) this is not actually all that bad - but it creates some inconsistencies between target allocation and target cleanup. I would rather keep all of this code in one place.

I did create an overwrite for applyOnlineStateChange. This method has different behavior for Multi-Tab clients and has no "mirrored" behavior in SyncEngine.

schmidt-sebastian · 2020-04-20T17:49:44Z

packages/firestore/src/core/component_provider.ts

@@ -210,6 +197,68 @@ export class MemoryLruComponentProvider extends MemoryComponentProvider {
 * Provides all components needed for Firestore with IndexedDB persistence.
 */
 export class IndexedDbComponentProvider extends MemoryComponentProvider {


Fixed (also renamed the constructor argument in FirestoreClient).

I opted not to go down the route and make this generic, but I did overwrite some of the members and use more explicit types. This gets rid of some of the asserts. Let me know if that is enough.

As part of the Master merge, I also had to fix the spec tests to be aware of the differences between the different persistence implementations.

schmidt-sebastian · 2020-04-20T20:33:56Z

I seem to have fixed all breakages. This is again ready for review.

rafikhan · 2020-04-20T21:15:03Z

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

-  constructor(private readonly queryEngine: QueryEngine) {}
+  constructor(
+    private readonly queryEngine: QueryEngine,
+    readonly type: 'index-free' | 'simple'


Would it make sense to create a type definition for this?

I added a test-only enum.

rafikhan · 2020-04-20T21:21:07Z

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

-    allowTabSynchronization: multiClient,
-    persistenceKey: TEST_PERSISTENCE_PREFIX,
+  const persistence = new IndexedDbPersistence(
+    /* allowTabSynchronization= */ multiClient,


Optional: Consider including a comment for sequenceNumberSyncer and persistenceKey or dropping this one. The inconsistency seems strange.

The goal of these comments is to make the argument type descriptive when just the value is not descriptive by default. In this case, one could argue that multiClient alone is descriptive enough. Dropped.

rafikhan · 2020-04-20T21:22:58Z

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

-    allowTabSynchronization: !!options.synchronizeTabs,
-    persistenceKey: TEST_PERSISTENCE_PREFIX,
+  const persistence = new IndexedDbPersistence(
+    !!options.synchronizeTabs,


This is another instance of my previous comment. You should comment this consistently.

I did not add the inline argument name here since I deemed synchronizeTabs more descriptive than multiTab. As stated above, both are probably descriptive enough on their own.

rafikhan

LGTM

schmidt-sebastian added 2 commits April 7, 2020 17:20

Factor out Multi-Tab code

73c5947

Merge

7178633

schmidt-sebastian force-pushed the mrschmidt/nomultitabv2 branch from bf2097f to 7178633 Compare April 8, 2020 01:07

schmidt-sebastian added the api: firestore label Apr 8, 2020

schmidt-sebastian requested a review from rafikhan April 8, 2020 01:17

schmidt-sebastian assigned rafikhan Apr 8, 2020

Merge

b7766c0

schmidt-sebastian force-pushed the mrschmidt/nomultitabv2 branch from ace33a9 to b7766c0 Compare April 9, 2020 22:18

Fix integration tests

2448b16

rafikhan requested changes Apr 18, 2020

View reviewed changes

schmidt-sebastian added 2 commits April 17, 2020 17:33

Cleanup

d9b0725

Refactor applyOnlineStateChange

7835818

schmidt-sebastian force-pushed the mrschmidt/nomultitabv2 branch from 6678110 to 7835818 Compare April 20, 2020 17:49

schmidt-sebastian commented Apr 20, 2020

View reviewed changes

schmidt-sebastian added 2 commits April 20, 2020 10:57

Merge

4eee9a1

Undo some of the cleanup

dc09fd1

schmidt-sebastian assigned schmidt-sebastian and unassigned rafikhan Apr 20, 2020

schmidt-sebastian added 2 commits April 20, 2020 12:53

MockIndexedDbPersistence

86ee9fa

[AUTOMATED]: Prettier Code Styling

fefc69c

schmidt-sebastian force-pushed the mrschmidt/nomultitabv2 branch from 7eeeeb5 to 86ee9fa Compare April 20, 2020 19:53

schmidt-sebastian assigned rafikhan and unassigned schmidt-sebastian Apr 20, 2020

rafikhan reviewed Apr 20, 2020

View reviewed changes

schmidt-sebastian added 3 commits April 20, 2020 16:52

Review

30a8251

Merge branch 'master' into mrschmidt/nomultitabv2

114be4b

[AUTOMATED]: Prettier Code Styling

a0bc0ce

schmidt-sebastian added 2 commits April 20, 2020 16:56

[AUTOMATED]: Prettier Code Styling

ba98a20

[AUTOMATED]: Prettier Code Styling

4c3e474

schmidt-sebastian force-pushed the mrschmidt/nomultitabv2 branch from 91b1a02 to ba98a20 Compare April 20, 2020 23:56

rafikhan approved these changes Apr 20, 2020

View reviewed changes

schmidt-sebastian added 2 commits April 20, 2020 18:29

Merge

7ef2d47

Fix merge

c4d6a84

schmidt-sebastian merged commit 5a60243 into master Apr 21, 2020

firebase locked and limited conversation to collaborators May 22, 2020

schmidt-sebastian deleted the mrschmidt/nomultitabv2 branch July 9, 2020 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out Multi-Tab code #2882

Factor out Multi-Tab code #2882

schmidt-sebastian commented Apr 8, 2020

google-oss-bot commented Apr 8, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/memory`

`firebase`

schmidt-sebastian commented Apr 9, 2020

rafikhan left a comment

rafikhan Apr 17, 2020

schmidt-sebastian Apr 20, 2020 •

edited

Loading

rafikhan Apr 18, 2020

schmidt-sebastian Apr 20, 2020

schmidt-sebastian left a comment

schmidt-sebastian Apr 20, 2020

schmidt-sebastian Apr 20, 2020 •

edited

Loading

schmidt-sebastian commented Apr 20, 2020

rafikhan Apr 20, 2020

schmidt-sebastian Apr 20, 2020

rafikhan Apr 20, 2020

schmidt-sebastian Apr 20, 2020

rafikhan Apr 20, 2020

schmidt-sebastian Apr 20, 2020

rafikhan left a comment

Factor out Multi-Tab code #2882

Factor out Multi-Tab code #2882

Conversation

schmidt-sebastian commented Apr 8, 2020

google-oss-bot commented Apr 8, 2020 • edited Loading

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/memory

firebase

Test Logs

schmidt-sebastian commented Apr 9, 2020

rafikhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian Apr 20, 2020 • edited Loading

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

schmidt-sebastian Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

schmidt-sebastian commented Apr 20, 2020

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

rafikhan left a comment

Choose a reason for hiding this comment

google-oss-bot commented Apr 8, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/memory`

`firebase`

schmidt-sebastian Apr 20, 2020 •

edited

Loading

schmidt-sebastian Apr 20, 2020 •

edited

Loading