Skip to content

Make View processing logic optional #3561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 24 commits into from
Aug 27, 2020
Merged

Make View processing logic optional #3561

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
aa4fc7f
Make View processing logic optional
schmidt-sebastian Aug 5, 2020
8350a84
Merge branch 'master' into mrschmidt/optionalviews
schmidt-sebastian Aug 5, 2020
dd872af
Merge branch 'master' into mrschmidt/optionalviews
schmidt-sebastian Aug 11, 2020
e2be013
Review
schmidt-sebastian Aug 11, 2020
eafa8e9
Make all of SyncEngine tree-shakeable
schmidt-sebastian Aug 14, 2020
03b1ce4
Re-run gendeps
schmidt-sebastian Aug 14, 2020
68f565b
Merge branch 'master' into mrschmidt/optionalviews
schmidt-sebastian Aug 14, 2020
4eccec3
Prettier
schmidt-sebastian Aug 14, 2020
f3efd44
Cleanup
schmidt-sebastian Aug 14, 2020
88a5bc0
Comment
schmidt-sebastian Aug 14, 2020
9f97606
Tree-shake all of LocalStore
schmidt-sebastian Aug 14, 2020
4b5105c
Comments
schmidt-sebastian Aug 14, 2020
9b018db
Merge
schmidt-sebastian Aug 14, 2020
c7d62f4
Merge
schmidt-sebastian Aug 15, 2020
08e425b
Undo EventManager's signature changes, use callbacks
schmidt-sebastian Aug 17, 2020
320d79a
Documentation
schmidt-sebastian Aug 17, 2020
a869f8f
Fix test
schmidt-sebastian Aug 17, 2020
d7b5d2a
Lint
schmidt-sebastian Aug 17, 2020
74ab415
Review
schmidt-sebastian Aug 21, 2020
452e460
Update tests
schmidt-sebastian Aug 21, 2020
1b2d3eb
Review/Fix build
schmidt-sebastian Aug 26, 2020
49bde61
More test fixes
schmidt-sebastian Aug 26, 2020
2674016
Undo async
schmidt-sebastian Aug 27, 2020
7f52b3c
Merge
schmidt-sebastian Aug 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
449 changes: 211 additions & 238 deletions packages/firestore/exp/dependencies.json
View file
Open in desktop

Large diffs are not rendered by default.

19 changes: 14 additions & 5 deletions packages/firestore/exp/src/api/components.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,14 @@ import {
OfflineComponentProvider,
OnlineComponentProvider
} from '../../../src/core/component_provider';
import {handleUserChange, LocalStore} from '../../../src/local/local_store';
import { handleUserChange, LocalStore } from '../../../src/local/local_store';
import { Deferred } from '../../../src/util/promise';
import { logDebug } from '../../../src/util/log';
import { SyncEngine } from '../../../src/core/sync_engine';
import {
SyncEngine,
syncEngineListen,
syncEngineUnlisten
} from '../../../src/core/sync_engine';
import { RemoteStore } from '../../../src/remote/remote_store';
import { Persistence } from '../../../src/local/persistence';
import { EventManager } from '../../../src/core/event_manager';
Expand Down Expand Up @@ -153,9 +157,14 @@ export function getRemoteStore(firestore: Firestore): Promise<RemoteStore> {
}

export function getEventManager(firestore: Firestore): Promise<EventManager> {
return getOnlineComponentProvider(firestore).then(
components => components.eventManager
);
return getOnlineComponentProvider(firestore).then(components => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const eventManager = components.eventManager;
eventManager.subscribe(
syncEngineListen.bind(null, components.syncEngine),
syncEngineUnlisten.bind(null, components.syncEngine)
);
return eventManager;
});
}

export function getPersistence(firestore: Firestore): Promise<Persistence> {
Expand Down
18 changes: 13 additions & 5 deletions packages/firestore/src/core/component_provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,11 @@ import {
import {
applyActiveTargetsChange,
applyBatchState,
applyOnlineStateChange,
applyPrimaryState,
applyTargetState,
getActiveClients,
handleCredentialChange,
newSyncEngine,
SyncEngine
} from './sync_engine';
Expand Down Expand Up @@ -332,20 +334,26 @@ export class OnlineComponentProvider {
this.syncEngine = this.createSyncEngine(cfg);
this.eventManager = this.createEventManager(cfg);

this.syncEngine.subscribe(this.eventManager);

this.sharedClientState.onlineStateHandler = onlineState =>
this.syncEngine.applyOnlineStateChange(
applyOnlineStateChange(
this.syncEngine,
onlineState,
OnlineStateSource.SharedClientState
);

this.remoteStore.syncEngine = this.syncEngine;
this.remoteStore.remoteSyncer.handleCredentialChange = handleCredentialChange.bind(
null,
this.syncEngine
);

await this.remoteStore.start();
await this.remoteStore.applyPrimaryState(this.syncEngine.isPrimaryClient);
}

createEventManager(cfg: ComponentConfiguration): EventManager {
return new EventManager(this.syncEngine);
return new EventManager();
}

createDatastore(cfg: ComponentConfiguration): Datastore {
Expand All @@ -360,7 +368,8 @@ export class OnlineComponentProvider {
this.datastore,
cfg.asyncQueue,
onlineState =>
this.syncEngine.applyOnlineStateChange(
applyOnlineStateChange(
this.syncEngine,
onlineState,
OnlineStateSource.RemoteStore
),
Expand All @@ -372,7 +381,6 @@ export class OnlineComponentProvider {
return newSyncEngine(
this.localStore,
this.remoteStore,
this.datastore,
this.sharedClientState,
cfg.initialUser,
cfg.maxConcurrentLimboResolutions,
Expand Down
25 changes: 20 additions & 5 deletions packages/firestore/src/core/event_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { debugAssert } from '../util/assert';
import { EventHandler } from '../util/misc';
import { ObjectMap } from '../util/obj_map';
import { canonifyQuery, Query, queryEquals, stringifyQuery } from './query';
import { SyncEngine, SyncEngineListener } from './sync_engine';
import { SyncEngineListener } from './sync_engine';
import { OnlineState } from './types';
import { ChangeType, DocumentViewChange, ViewSnapshot } from './view_snapshot';
import { wrapInUserErrorIfRecoverable } from '../util/async_queue';
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

* register SyncEngine's `listen()` and `unlisten()` functionality. This allows
* users to tree-shake the Watch logic.
*/
export class EventManager implements SyncEngineListener {
private queries = new ObjectMap<Query, QueryListenersInfo>(
Expand All @@ -56,11 +60,21 @@ export class EventManager implements SyncEngineListener {

private snapshotsInSyncListeners: Set<Observer<void>> = new Set();

constructor(private syncEngine: SyncEngine) {
this.syncEngine.subscribe(this);
/** Callback invoked when a Query is first listen to. */
private onListen?: (query: Query) => Promise<ViewSnapshot>;
/** Callback invoked once all listeners to a Query are removed. */
private onUnlisten?: (query: Query) => Promise<void>;

subscribe(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

onListen: (query: Query) => Promise<ViewSnapshot>,
onUnlisten: (query: Query) => Promise<void>
): void {
this.onListen = onListen;
this.onUnlisten = onUnlisten;
}

async listen(listener: QueryListener): Promise<void> {
debugAssert(!!this.onListen, 'onListen not set');
const query = listener.query;
let firstListen = false;

Expand All @@ -72,7 +86,7 @@ export class EventManager implements SyncEngineListener {

if (firstListen) {
try {
queryInfo.viewSnap = await this.syncEngine.listen(query);
queryInfo.viewSnap = await this.onListen(query);
} catch (e) {
const firestoreError = wrapInUserErrorIfRecoverable(
e,
Expand Down Expand Up @@ -102,6 +116,7 @@ export class EventManager implements SyncEngineListener {
}

async unlisten(listener: QueryListener): Promise<void> {
debugAssert(!!this.onUnlisten, 'onUnlisten not set');
const query = listener.query;
let lastListen = false;

Expand All @@ -116,7 +131,7 @@ export class EventManager implements SyncEngineListener {

if (lastListen) {
this.queries.delete(query);
return this.syncEngine.unlisten(query);
return this.onUnlisten(query);
}
}

Expand Down
31 changes: 22 additions & 9 deletions packages/firestore/src/core/firestore_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,13 @@ import {
Observer,
QueryListener
} from './event_manager';
import { SyncEngine } from './sync_engine';
import {
registerPendingWritesCallback,
SyncEngine,
syncEngineListen,
syncEngineUnlisten,
syncEngineWrite
} from './sync_engine';
import { View } from './view';
import { SharedClientState } from '../local/shared_client_state';
import { AutoId } from '../util/misc';
Expand Down Expand Up @@ -277,6 +283,11 @@ export class FirestoreClient {
this.syncEngine = onlineComponentProvider.syncEngine;
this.eventMgr = onlineComponentProvider.eventManager;

this.eventMgr.subscribe(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

syncEngineListen.bind(null, this.syncEngine),
syncEngineUnlisten.bind(null, this.syncEngine)
);

// When a user calls clearPersistence() in one client, all other clients
// need to be terminated to allow the delete to succeed.
this.persistence.setDatabaseDeletedListener(async () => {
Expand Down Expand Up @@ -405,9 +416,9 @@ export class FirestoreClient {
this.verifyNotTerminated();

const deferred = new Deferred<void>();
this.asyncQueue.enqueueAndForget(() => {
return this.syncEngine.registerPendingWritesCallback(deferred);
});
this.asyncQueue.enqueueAndForget(() =>
registerPendingWritesCallback(this.syncEngine, deferred)
);
return deferred.promise;
}

Expand Down Expand Up @@ -480,7 +491,7 @@ export class FirestoreClient {
this.verifyNotTerminated();
const deferred = new Deferred<void>();
this.asyncQueue.enqueueAndForget(() =>
this.syncEngine.write(mutations, deferred)
syncEngineWrite(this.syncEngine, mutations, deferred)
);
return deferred.promise;
}
Expand Down Expand Up @@ -549,7 +560,9 @@ export function enqueueWrite(
mutations: Mutation[]
): Promise<void> {
const deferred = new Deferred<void>();
asyncQueue.enqueueAndForget(() => syncEngine.write(mutations, deferred));
asyncQueue.enqueueAndForget(() =>
syncEngineWrite(syncEngine, mutations, deferred)
);
return deferred.promise;
}

Expand All @@ -570,9 +583,9 @@ export function enqueueWaitForPendingWrites(
syncEngine: SyncEngine
): Promise<void> {
const deferred = new Deferred<void>();
asyncQueue.enqueueAndForget(() => {
return syncEngine.registerPendingWritesCallback(deferred);
});
asyncQueue.enqueueAndForget(() =>
registerPendingWritesCallback(syncEngine, deferred)
);
return deferred.promise;
}

Expand Down
Loading