Make View processing logic optional #3561

schmidt-sebastian · 2020-08-04T22:37:17Z

This PR changes EventManager and SyncEngine to make listen/unlisten and the related View-processing logic optional. This is a stepping stone that will allow us to remove any Watch-related code for users that don't use Watch (or let them lazy load the components).

View processing logic is "injected" via a new docsChangeListener in SyncEngine that is provided when the first view is initialized: https://github.com/firebase/firebase-js-sdk/pull/3561/files#diff-50ed4637ed85c4c9895df1ce8112114aR867
I personally think this is pretty ugly, but I couldn't come up with a cleaner design. Suggestions welcome :)

changeset-bot · 2020-08-04T22:37:19Z

💥 No Changeset

Latest commit: 7f52b3c

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

google-oss-bot · 2020-08-04T22:48:57Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`e8b950f`)	Head (`b2e9ad5`)	Diff
browser	249 kB	249 kB	-163 B (-0.1%)
esm2017	195 kB	195 kB	+238 B (+0.1%)
main	475 kB	478 kB	+2.91 kB (+0.6%)
module	246 kB	246 kB	-185 B (-0.1%)
react-native	195 kB	196 kB	+238 B (+0.1%)

`@firebase/firestore/exp`

Type	Base (`e8b950f`)	Head (`b2e9ad5`)	Diff
browser	189 kB	189 kB	+245 B (+0.1%)
main	468 kB	471 kB	+2.95 kB (+0.6%)
module	189 kB	189 kB	+245 B (+0.1%)
react-native	189 kB	189 kB	+245 B (+0.1%)

`@firebase/firestore/memory`

Type	Base (`e8b950f`)	Head (`b2e9ad5`)	Diff
browser	187 kB	186 kB	-182 B (-0.1%)
esm2017	146 kB	147 kB	+210 B (+0.1%)
main	349 kB	352 kB	+2.82 kB (+0.8%)
module	185 kB	184 kB	-204 B (-0.1%)
react-native	146 kB	147 kB	+210 B (+0.1%)

`firebase`

Type	Base (`e8b950f`)	Head (`b2e9ad5`)	Diff
firebase-firestore.js	287 kB	287 kB	-307 B (-0.1%)
firebase-firestore.memory.js	227 kB	227 kB	-379 B (-0.2%)
firebase.js	822 kB	822 kB	-305 B (-0.0%)

Test Logs

Base (e8b950fd): https://github.com/firebase/firebase-js-sdk/actions/runs/209877208
Head (b2e9ad5f): https://github.com/firebase/firebase-js-sdk/actions/runs/212124307

wilhuff

This would be significantly easier to review if you made the structural changes related to tree shaking separately from the logic changes required to make view processing separable.

On the whole this doesn't seem awful, but I think we'd benefit from having a standard, concise way of describing demand-loaded types without relying on interfaces and classes implementing interfaces. I've made some concrete suggestions below.

wilhuff · 2020-08-07T20:03:09Z

packages/firestore/src/core/sync_engine.ts

+   * `emitNewSnapshotsAndNotifyLocalStore()`, but on Web it is extracted to
+   *  ensure that all view logic only exists in bundles that include views.
+   */
+  docsChangeListener:


Since this is essentially just a way to make an indirection between emitNewSnapsAndNotifyLocalStore and the actual implementation is always going to be applyDocChanges, why not just call this applyDocChanges? This avoids the indirection through a more-general-than-necessary concept and makes it easier to read through it.

You might also consider having a noopApplyDocChanges that's assigned here by default. That gives you two things: you can lose the null check and you can make the type just typeof noopApplyDocChanges, though I wonder if you could make it typeof applyDocChanges without incurring the bundling penalty.

Failing either of those things, a named type could make this easier to understand and reason about: if you named it an ApplyDocChangesHandler, this would be

applyDocChanges: ApplyDocChangesHandler | null

... which more directly names what's expected.

I renamed this to applyDocChanges and created a type definition. I ended up leaving this listener as optional as this allows me to verify that the code is injected by the time it is used.

wilhuff · 2020-08-07T20:13:53Z

packages/firestore/src/core/sync_engine.ts

      queryView.targetId,
      viewSnapshot.limboChanges
    );
  }
  return viewSnapshot;
 }

+/**


The old ordering of listen, initializeViewAndComputeSnapshot, then applyRemoteEvent reads better because it followed the logical sequence of events you might encounter at runtime. This new ordering isn't alphabetical and doesn't seem to have any rhyme or reason to it that I could find.

The ordering is mostly "organic". I tried to keep public methods on top, but probably failed a couple times (and the multi-tab methods are still grouped together at the bottom of this file). I re-ordered them as suggested.

wilhuff · 2020-08-07T20:17:27Z

packages/firestore/src/core/event_manager.ts

+import {
+  SyncEngine,
+  SyncEngineListener,
+  listen as syncEngineListen,


I'm not a fan of this kind of renaming internally. It makes it hard to search the codebase textually.

I renamed the original functions.

wilhuff · 2020-08-07T20:32:45Z

packages/firestore/src/core/sync_engine.ts

+      .then(() => removeAndCleanupTarget(syncEngineImpl, targetId, err))
+      .catch(ignoreIfPrimaryLeaseLoss);
+  }
+}


Missing newline.

wilhuff · 2020-08-07T20:42:22Z

packages/firestore/src/core/sync_engine.ts

+  return viewChange.snapshot;
+}
+
+async function applyDocChanges(


Optional: consider grouping this immediately below emitNewSnapsAndNotifyLocalStore since that's the logical flow (even though the linkage is indirect). The suggestion is based on the idea that logical flow is more important than grouping this with the function that happens to install it in the sync engine (which seems arbitrary).

emitNewSnapsAndNotifyLocalStore is not a free standing function (yet). It likely won't need to be one for size savings (since most codepaths rely on it), but we can make it one for consistency.

We might as well just adopt the one style so you don't have to think about whether a given operation is a method or a free function.

wilhuff · 2020-08-07T21:00:12Z

packages/firestore/src/remote/remote_store.ts

@@ -55,6 +55,7 @@ import {
 import { ByteString } from '../util/byte_string';
 import { isIndexedDbTransactionError } from '../local/simple_db';
 import { User } from '../auth/user';
+import { rejectListen, applyRemoteEvent } from '../core/sync_engine';


This creates a strong cycle between the SyncEngine and the RemoteStore (and also breaks our ability to inject an alternate implementation for testing, though I don't think we've ever done that).

Could you leave the RemoteSyncer interface as having those two methods, and then make function pointers of those names members of SyncEngine so that it still implements the interface, just not via its prototype?

I thought long about this (especially since this change made the mocking in the EventListener test much more complex). I don' think there is a way to achieve the code savings that I am after if we use one shared interface.

Instead, we would have to split RemoteSyncer into at least two interfaces (one for write handling, one for watch). On top of that, we would also need to inject the actual implementation at runtime, similar to what we are now doing in SyncEngine. If we provided the implementation at instantiation, the code would be included no matter what.

Don't we have to solve the cycle problem though? Given all the trouble we've been having with random reorderings breaking things it seems unwise for us to intentionally create such cycles.

Meanwhile, what I'm proposing is that we have RemoteStore have a RemoteSyncer member whose members are fields that can be assigned. Then in the syncEngine once we get to a place where those callbacks are going to be required in the RemoteStore we assign to the fields. This gives essentially the same split you get here while preserving the flexibility in tests.

schmidt-sebastian

I updated the PR to move all SyncEngine logic into free-standing functions. This makes the diff even larger, but it is actually easier to review (especially in IntelliJ, which makes it pretty easy to figure out the code that actually changed). I also re-added the RemoteSyncer interface.

schmidt-sebastian · 2020-08-17T14:59:44Z

I wasn't able to come up with a way to get the unit tests for EventManager to pass without adding another interface that let's me replace the sync engine callbacks with custom test functions. If I don't do that, I have to bring up a full instance of SyncEngine and a full instance of LocalStore, which requires mocking all of persistence. Assigning sync engine's listen() and unlisten() callbacks at startup breaks tree-shaking again, so I made a follow up to address this: #3640

wilhuff

Basically LGTM though with some final questions.

wilhuff · 2020-08-21T17:50:05Z

packages/firestore/exp/src/api/components.ts

-  return getOnlineComponentProvider(firestore).then(
-    components => components.eventManager
-  );
+  return getOnlineComponentProvider(firestore).then(components => {


No need to address this here, necessarily, but why not write these with async/await?

I don't remember why I did it this way, but "await" here is much better, especially now that the function has some nested logic. Change is pretty easy and only in 5 places so I did it just now.

wilhuff · 2020-08-21T18:00:44Z

packages/firestore/src/core/event_manager.ts

+  /** Callback invoked once all listeners to a Query are removed. */
+  private onUnlisten?: (query: Query) => Promise<void>;
+
+  subscribe(


The name "subscribe" is confusing in this context because this doesn't actually subscribe, it only registers callbacks. Also have listen operations so it's potentially confusing to have to choose between listen and subscribe.

Meanwhile, in other contexts where we're wiring up these kinds of things on demand there is no separate function: we're just assigning to the members to set them. Rather than renaming this to something like "registerHandlers" or something like that, why not remove this function and assign directly?

That works for me. I changed it to assign directly. The code size change will be part of my next perf :)

Update: I had to revert this since this breaks the "terminate()" tests. The call to get the component provider throws an exception if the client has already been terminated, and converting these calls to async functions turns that into a rejected Promise.

wilhuff · 2020-08-21T18:03:05Z

packages/firestore/src/core/firestore_client.ts

@@ -277,6 +283,11 @@ export class FirestoreClient {
      this.syncEngine = onlineComponentProvider.syncEngine;
      this.eventMgr = onlineComponentProvider.eventManager;

+      this.eventMgr.subscribe(


Doesn't doing this unconditionally here cause this to not be tree-shakeable? I would have thought we'd do this in the listen method.

Because of how FirestoreClient brings together all components, I actually removed FirestoreClient from the firestore-exp build to support the the cache-only build. It will eventually go away altogether, but for now it is still needed for the legacy build.

In the modular build, these functions are only assigned in getEventManager, which is only called in the listen codepath.

schmidt-sebastian · 2020-08-26T14:56:03Z

@wilhuff In case you need a distraction :)

wilhuff

LGTM

wilhuff · 2020-08-26T16:38:23Z

packages/firestore/src/core/event_manager.ts

@@ -45,6 +45,10 @@ export interface Observer<T> {
 * EventManager is responsible for mapping queries to query event emitters.
 * It handles "fan-out". -- Identical queries will re-use the same watch on the
 * backend.
+ *
+ * PORTING NOTE: On Web, EventManager requires a call to `subscribe()` to


This comment is no longer accurate. Instead, the onListen and onUnlisten handlers need to be assigned.

google-oss-bot · 2020-08-26T17:58:04Z

Size Analysis Report

Affected Products

No changes between base commit (e22a295) and head commit (0264f3a).

Test Logs

Base (e22a2957): https://github.com/firebase/firebase-js-sdk/actions/runs/226795650
Head (0264f3aa): https://github.com/firebase/firebase-js-sdk/actions/runs/226809016

yuchenshi

I approve the semicolon change in rules-unit-testing;
Ship it;

No; I did not review anything else;

Make View processing logic optional

aa4fc7f

schmidt-sebastian force-pushed the mrschmidt/optionalviews branch from fceba12 to aa4fc7f Compare August 5, 2020 06:08

schmidt-sebastian changed the title ~~Mrschmidt/optionalviews~~ Make View processing logic optional Aug 5, 2020

schmidt-sebastian requested a review from wilhuff August 5, 2020 16:39

schmidt-sebastian assigned wilhuff Aug 5, 2020

schmidt-sebastian added the api: firestore label Aug 5, 2020

Merge branch 'master' into mrschmidt/optionalviews

8350a84

wilhuff reviewed Aug 7, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Aug 7, 2020

schmidt-sebastian added 2 commits August 11, 2020 21:10

Merge branch 'master' into mrschmidt/optionalviews

dd872af

Review

e2be013

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Aug 11, 2020

wilhuff assigned schmidt-sebastian and unassigned wilhuff Aug 12, 2020

schmidt-sebastian added 5 commits August 14, 2020 11:47

Make all of SyncEngine tree-shakeable

eafa8e9

Re-run gendeps

03b1ce4

Merge branch 'master' into mrschmidt/optionalviews

68f565b

Prettier

4eccec3

Cleanup

f3efd44

schmidt-sebastian commented Aug 14, 2020

View reviewed changes

Comment

88a5bc0

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Aug 14, 2020

Tree-shake all of LocalStore

9f97606

schmidt-sebastian mentioned this pull request Aug 14, 2020

Tree-shake LocalStore's implementation #3531

Merged

Comments

4b5105c

schmidt-sebastian added 6 commits August 14, 2020 19:15

Merge

9b018db

Merge

c7d62f4

Undo EventManager's signature changes, use callbacks

08e425b

Documentation

320d79a

Fix test

a869f8f

Lint

d7b5d2a

schmidt-sebastian mentioned this pull request Aug 17, 2020

Tree-Shake EventManager #3640

Merged

wilhuff reviewed Aug 21, 2020

View reviewed changes

schmidt-sebastian added 2 commits August 21, 2020 23:44

Review

74ab415

Update tests

452e460

wilhuff approved these changes Aug 26, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Aug 26, 2020

Review/Fix build

1b2d3eb

schmidt-sebastian added 3 commits August 26, 2020 20:55

More test fixes

49bde61

Undo async

2674016

Merge

7f52b3c

schmidt-sebastian requested review from avolkovi, sam-gc, scottcrossen and yuchenshi as code owners August 27, 2020 09:59

yuchenshi approved these changes Aug 27, 2020

View reviewed changes

schmidt-sebastian merged commit 29bac5e into master Aug 27, 2020

firebase locked and limited conversation to collaborators Sep 27, 2020

schmidt-sebastian deleted the mrschmidt/optionalviews branch November 9, 2020 22:36

Make View processing logic optional #3561

Make View processing logic optional #3561

Conversation

schmidt-sebastian commented Aug 4, 2020 • edited Loading

changeset-bot bot commented Aug 4, 2020 • edited Loading

💥 No Changeset

google-oss-bot commented Aug 4, 2020 • edited Loading

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/exp

@firebase/firestore/memory

firebase

Test Logs

wilhuff 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

schmidt-sebastian left a comment

Choose a reason for hiding this comment

schmidt-sebastian commented Aug 17, 2020

wilhuff 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 Aug 26, 2020

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-bot commented Aug 26, 2020 • edited Loading

Size Analysis Report

Affected Products

Test Logs

yuchenshi left a comment

Choose a reason for hiding this comment

schmidt-sebastian commented Aug 4, 2020 •

edited

Loading

changeset-bot bot commented Aug 4, 2020 •

edited

Loading

google-oss-bot commented Aug 4, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/memory`

`firebase`

google-oss-bot commented Aug 26, 2020 •

edited

Loading