Skip to content

Bundles load from secondary tabs #3393

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.

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 34 commits into from
Jul 23, 2020
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
9602712
Renaming interfaces without leading I
wu-hui May 15, 2020
c5e783e
Initial commit of bundle reading - for web only.
wu-hui May 21, 2020
5e7fb89
Tests only run when it is not Node.
wu-hui May 21, 2020
1ee1615
Fix redundant imports
wu-hui May 21, 2020
18f0be1
Fix missing textencoder
wu-hui May 21, 2020
aa455bf
Remove generator.
wu-hui May 29, 2020
78248cd
Support bundle reader for Node
wu-hui May 23, 2020
83160a1
Fix rebase errors.
wu-hui May 29, 2020
24e10cb
Remote 'only'
wu-hui May 29, 2020
9d6edc5
Merge branch 'wuandy/Bundles' into wuandy/BundleReaderNode
wu-hui Jun 5, 2020
4cbe608
Merge branch 'wuandy/Bundles' into wuandy/BundleReaderNode
wu-hui Jun 5, 2020
4313e51
Added more comments, and more tests for Node.
wu-hui Jun 5, 2020
296cfc4
Implement BundleCache.
wu-hui Jun 5, 2020
fff3d36
Add applyBundleDocuments to local store.
wu-hui Jun 1, 2020
fb762de
Add rest of bundle service to localstore
wu-hui Jun 1, 2020
1ec4182
Simplify change buffer get read time logic.
wu-hui Jun 2, 2020
cd3ab7a
Fix lint errors
wu-hui Jun 2, 2020
d991c75
Add comments.
wu-hui Jun 2, 2020
af097c5
Change localstore to check for newer bundle directly.
wu-hui Jun 2, 2020
e735e23
temp checkin
wu-hui Jun 3, 2020
17ba434
Implement without async/await
wu-hui Jun 4, 2020
f808d8d
Major code complete.
wu-hui Jun 4, 2020
db1d864
Integration tests added.
wu-hui Jun 6, 2020
979ffd9
Added spec tests.
wu-hui Jun 9, 2020
f7ff495
Add comments and move types to d.ts
wu-hui Jun 9, 2020
556a007
Support loading string for real.
wu-hui Jun 9, 2020
adf1504
Makes sure SDK still works after loading bad bundles.
wu-hui Jun 11, 2020
b364ab0
Better spect test.
wu-hui Jun 12, 2020
2588f20
Load bundles from secondary clients should raise snapshots
wu-hui Jun 13, 2020
93bc964
Support secondary to secondary.
wu-hui Jun 13, 2020
99610d7
Merge branch 'wuandy/Bundles' into wuandy/BundlesLoadFromSecondary
wu-hui Jul 11, 2020
e18493a
Address comments.
wu-hui Jul 22, 2020
d175a14
Address more comments.
wu-hui Jul 23, 2020
5a4d0f2
Move newTextEncoder to serializer.ts
wu-hui Jul 23, 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
3 changes: 2 additions & 1 deletion packages/firestore/src/core/firestore_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import { LoadBundleTask } from '../api/bundle';
import { newConnection } from '../platform/connection';
import { newSerializer } from '../platform/serializer';
import { toByteStreamReader } from '../platform/byte_stream_reader';
import { newTextEncoder } from '../platform/dom';

const LOG_TAG = 'FirestoreClient';
const MAX_CONCURRENT_LIMBO_RESOLUTIONS = 100;
Expand Down Expand Up @@ -529,7 +530,7 @@ export class FirestoreClient {

let content: ReadableStream<Uint8Array> | ArrayBuffer;
if (typeof data === 'string') {
content = new TextEncoder().encode(data);
content = newTextEncoder().encode(data);
} else {
content = data;
}
Expand Down
12 changes: 10 additions & 2 deletions packages/firestore/src/core/sync_engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ class SyncEngineImpl implements SyncEngine {
protected remoteStore: RemoteStore,
protected datastore: Datastore,
// PORTING NOTE: Manages state synchronization in multi-tab environments.
protected sharedClientState: SharedClientState,
public sharedClientState: SharedClientState,
private currentUser: User,
private maxConcurrentLimboResolutions: number
) {}
Expand Down Expand Up @@ -1100,6 +1100,12 @@ class MultiTabSyncEngineImpl extends SyncEngineImpl {
}
}

synchronizeWithChangedDocuments(): Promise<void> {
return this.localStore
.getNewDocumentChanges()
.then(changes => this.emitNewSnapsAndNotifyLocalStore(changes));
}

async applyBatchState(
batchId: BatchId,
batchState: MutationBatchState,
Expand Down Expand Up @@ -1412,7 +1418,9 @@ export function loadBundle(
syncEngineImpl.assertSubscribed('loadBundle()');

// eslint-disable-next-line @typescript-eslint/no-floating-promises
loadBundleImpl(syncEngineImpl, bundleReader, task);
loadBundleImpl(syncEngineImpl, bundleReader, task).then(() => {
syncEngineImpl.sharedClientState.notifyBundleChangedRemoteDocuments();
});
}

async function loadBundleImpl(
Expand Down
26 changes: 26 additions & 0 deletions packages/firestore/src/local/shared_client_state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
import {
CLIENT_STATE_KEY_PREFIX,
ClientStateSchema,
createRemoteDocumentsLoadFromBundleKey,
createWebStorageClientStateKey,
createWebStorageMutationBatchKey,
createWebStorageOnlineStateKey,
Expand Down Expand Up @@ -173,6 +174,12 @@ export interface SharedClientState {
setOnlineState(onlineState: OnlineState): void;

writeSequenceNumber(sequenceNumber: ListenSequenceNumber): void;

/**
* Notifies other clients when remote documents have changed due to loading
* a bundle.
*/
notifyBundleChangedRemoteDocuments(): void;
}

/**
Expand Down Expand Up @@ -477,6 +484,7 @@ export class WebStorageSharedClientState implements SharedClientState {
private readonly sequenceNumberKey: string;
private readonly storageListener = this.handleWebStorageEvent.bind(this);
private readonly onlineStateKey: string;
private readonly bundleChangedRemoteDocumentsKey: string;
private readonly clientStateKeyRe: RegExp;
private readonly mutationBatchKeyRe: RegExp;
private readonly queryTargetKeyRe: RegExp;
Expand Down Expand Up @@ -532,6 +540,10 @@ export class WebStorageSharedClientState implements SharedClientState {

this.onlineStateKey = createWebStorageOnlineStateKey(this.persistenceKey);

this.bundleChangedRemoteDocumentsKey = createRemoteDocumentsLoadFromBundleKey(
Copy link
Contributor

Choose a reason for hiding this comment

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

createRemoteDocumentsLoadFromBundleKey seems like it needs a name update now as well. createBundleLoadedKey?

For sake of brevity, I would also change bundleChangedRemoteDocumentsKey to bundleLoadedKey. I seems shorter and more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

this.persistenceKey
);

// Rather than adding the storage observer during start(), we add the
// storage observer during initialization. This ensures that we collect
// events before other components populate their initial state (during their
Expand Down Expand Up @@ -711,6 +723,10 @@ export class WebStorageSharedClientState implements SharedClientState {
this.persistOnlineState(onlineState);
}

notifyBundleChangedRemoteDocuments(): void {
this.persistBundleChangedRemoteDocumentsState();
}

shutdown(): void {
if (this.started) {
this.window.removeEventListener('storage', this.storageListener);
Expand Down Expand Up @@ -818,6 +834,8 @@ export class WebStorageSharedClientState implements SharedClientState {
if (sequenceNumber !== ListenSequence.INVALID) {
this.sequenceNumberHandler!(sequenceNumber);
}
} else if (storageEvent.key === this.bundleChangedRemoteDocumentsKey) {
return this.syncEngine!.synchronizeWithChangedDocuments();
}
});
}
Expand Down Expand Up @@ -883,6 +901,10 @@ export class WebStorageSharedClientState implements SharedClientState {
this.setItem(targetKey, targetMetadata.toWebStorageJSON());
}

private persistBundleChangedRemoteDocumentsState(): void {
this.setItem(this.bundleChangedRemoteDocumentsKey, 'value-not-used');
}

/**
* Parses a client state key in WebStorage. Returns null if the key does not
* match the expected key format.
Expand Down Expand Up @@ -1131,4 +1153,8 @@ export class MemorySharedClientState implements SharedClientState {
shutdown(): void {}

writeSequenceNumber(sequenceNumber: ListenSequenceNumber): void {}

notifyBundleChangedRemoteDocuments(): void {
// No op.
}
}
12 changes: 12 additions & 0 deletions packages/firestore/src/local/shared_client_state_schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,18 @@ export function createWebStorageOnlineStateKey(persistenceKey: string): string {
return `${ONLINE_STATE_KEY_PREFIX}_${persistenceKey}`;
}

// The WebStorage prefix that plays as a event to indicate the remote documents
// might have changed due to some secondary tabs loading a bundle.
// format of the key is:
// firestore_remote_documents_changed_<persistenceKey>
export const REMOTE_DOCUMENTS_LOAD_FROM_BUNDLE_KEY_PREFIX =
'firestore_remote_documents_changed';
Copy link
Contributor

Choose a reason for hiding this comment

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

So the remote documents change when queries get updated and even when mutations are committed, but this even only fires when a bundle is loaded. Should this be a bundle specific notification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also update the key prefix. For sake of brevity, you could consider dropping "remote documents" altogether and just make it something like "bundle loaded".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

export function createRemoteDocumentsLoadFromBundleKey(
persistenceKey: string
): string {
return `${REMOTE_DOCUMENTS_LOAD_FROM_BUNDLE_KEY_PREFIX}_${persistenceKey}`;
}

/**
* The JSON representation of the system's online state, as written by the
* primary client.
Expand Down
6 changes: 6 additions & 0 deletions packages/firestore/src/local/shared_client_state_syncer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,10 @@ export interface SharedClientStateSyncer {

/** Returns the IDs of the clients that are currently active. */
getActiveClients(): Promise<ClientId[]>;

/**
* Retrieves newly changed documents from remote document cache and raises
* snapshots if needed.
*/
synchronizeWithChangedDocuments(): Promise<void>;
}
14 changes: 14 additions & 0 deletions packages/firestore/src/platform/browser/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,17 @@ export function getDocument(): Document | null {
// eslint-disable-next-line no-restricted-globals
return typeof document !== 'undefined' ? document : null;
}

/**
* An instance of the Platform's 'TextEncoder' implementation.
*/
export function newTextEncoder(): TextEncoder {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should only be in platform is there is some divergence between platforms. This doesn't seem to be the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yarn test:node complains there is no TextEncoder, it looks like it has to be imported from util.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sad :/

Should we move this to the ./serializer.ts file though? It seems much more fitting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but it is not used in serializer.ts. The only place it is used in non-testing code is util/bundle_reader.ts, I think it makes slightly more sense to keep it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean to move it from packages/firestore/src/platform/browser/dom.ts to packages/firestore/src/platform/browser/serializer.ts. It has little to do with DOM, mostly because it exists in Node which doesn't have DOM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry. Done.

return new TextEncoder();
}

/**
* An instance of the Platform's 'TextDecoder' implementation.
*/
export function newTextDecoder(): TextDecoder {
return new TextDecoder('utf-8');
}
26 changes: 26 additions & 0 deletions packages/firestore/src/platform/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,29 @@ export function getDocument(): Document | null {
return browser.getDocument();
}
}

/**
* An instance of the Platform's 'TextEncoder' implementation.
*/
export function newTextEncoder(): TextEncoder {
if (isNode()) {
return node.newTextEncoder();
} else if (isReactNative()) {
return rn.newTextEncoder();
} else {
return browser.newTextEncoder();
}
}

/**
* An instance of the Platform's 'TextDecoder' implementation.
*/
export function newTextDecoder(): TextDecoder {
if (isNode()) {
return node.newTextDecoder() as TextDecoder;
} else if (isReactNative()) {
return rn.newTextDecoder();
} else {
return browser.newTextDecoder();
}
}
16 changes: 16 additions & 0 deletions packages/firestore/src/platform/node/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
* limitations under the License.
*/

import { TextEncoder, TextDecoder } from 'util';

/** The Platform's 'window' implementation or null if not available. */
export function getWindow(): Window | null {
if (process.env.USE_MOCK_PERSISTENCE === 'YES') {
Expand All @@ -29,3 +31,17 @@ export function getWindow(): Window | null {
export function getDocument(): Document | null {
return null;
}

/**
* An instance of the Platform's 'TextEncoder' implementation.
*/
export function newTextEncoder(): TextEncoder {
return new TextEncoder();
}

/**
* An instance of the Platform's 'TextDecoder' implementation.
*/
export function newTextDecoder(): TextDecoder {
return new TextDecoder('utf-8');
}
7 changes: 6 additions & 1 deletion packages/firestore/src/platform/rn/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,9 @@
* limitations under the License.
*/

export { getWindow, getDocument } from '../browser/dom';
export {
getWindow,
getDocument,
newTextEncoder,
newTextDecoder
} from '../browser/dom';
9 changes: 6 additions & 3 deletions packages/firestore/src/util/bundle_reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
import { Deferred } from './promise';
import { debugAssert } from './assert';
import { toByteStreamReader } from '../platform/byte_stream_reader';
import { newTextDecoder } from '../platform/dom';

/**
* A complete element in the bundle stream, together with the byte length it
Expand Down Expand Up @@ -67,7 +68,7 @@ export class BundleReader {
*/
private buffer: Uint8Array = new Uint8Array();
/** The decoder used to parse binary data into strings. */
private textDecoder = new TextDecoder('utf-8');
private textDecoder: TextDecoder | null;

static fromBundleSource(source: BundleSource): BundleReader {
return new BundleReader(toByteStreamReader(source, BYTES_PER_READ));
Expand All @@ -77,6 +78,8 @@ export class BundleReader {
/** The reader to read from underlying binary bundle data source. */
private reader: ReadableStreamReader<Uint8Array>
) {
this.textDecoder = newTextDecoder();
debugAssert(!!this.textDecoder, 'Cannot create a valid text decoder.');
Copy link
Contributor

Choose a reason for hiding this comment

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

This would never fire, would it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, fixed.

// Read the metadata (which is the first element).
this.nextElementImpl().then(
element => {
Expand Down Expand Up @@ -131,7 +134,7 @@ export class BundleReader {
return null;
}

const lengthString = this.textDecoder.decode(lengthBuffer);
const lengthString = this.textDecoder!.decode(lengthBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop bang.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

const length = Number(lengthString);
if (isNaN(length)) {
this.raiseError(`length string (${lengthString}) is not valid number`);
Expand Down Expand Up @@ -199,7 +202,7 @@ export class BundleReader {
}
}

const result = this.textDecoder.decode(this.buffer.slice(0, length));
const result = this.textDecoder!.decode(this.buffer.slice(0, length));
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop bang.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// Update the internal buffer to drop the read json string.
this.buffer = this.buffer.slice(length);
return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ import { DatabaseId } from '../../../src/core/database_info';
import { key } from '../../util/helpers';
import { EventsAccumulator } from '../util/events_accumulator';
import { TestBundleBuilder } from '../../unit/util/bundle_data';
import { newTextEncoder } from '../../../src/platform/dom';

export const encoder = newTextEncoder();

function verifySuccessProgress(p: firestore.LoadBundleTaskProgress): void {
expect(p.taskState).to.equal('Success');
Expand All @@ -45,7 +48,6 @@ function verifyInProgress(
}

apiDescribe('Bundles', (persistence: boolean) => {
const encoder = new TextEncoder();
const testDocs: { [key: string]: firestore.DocumentData } = {
a: { k: { stringValue: 'a' }, bar: { integerValue: 1 } },
b: { k: { stringValue: 'b' }, bar: { integerValue: 2 } }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ class NoOpSharedClientStateSyncer implements SharedClientStateSyncer {
removed: TargetId[]
): Promise<void> {}
applyOnlineStateChange(onlineState: OnlineState): void {}

async synchronizeWithChangedDocuments(): Promise<void> {}
}
/**
* Populates Web Storage with instance data from a pre-existing client.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@ class TestSharedClientSyncer implements SharedClientStateSyncer {
applyOnlineStateChange(onlineState: OnlineState): void {
this.onlineState = onlineState;
}

async synchronizeWithChangedDocuments(): Promise<void> {}
}

describe('WebStorageSharedClientState', () => {
Expand Down
9 changes: 3 additions & 6 deletions packages/firestore/test/unit/specs/bundle_spec.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -293,12 +293,9 @@ describeSpec('Bundles:', ['no-ios', 'no-android'], () => {
.client(1)
.loadBundle(bundleString1)
.client(0)
.becomeVisible();
// TODO(wuandy): Loading from secondary client does not notify other
// clients for now. We need to fix it and uncomment below.
// .expectEvents(query, {
// modified: [doc('collection/a', 500, { value: 'b' })],
// })
.expectEvents(query, {
modified: [doc('collection/a', 500, { value: 'b' })]
});
}
);

Expand Down
3 changes: 2 additions & 1 deletion packages/firestore/test/unit/specs/spec_test_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ import {
} from '../../util/test_platform';
import { toByteStreamReader } from '../../../src/platform/byte_stream_reader';
import { logWarn } from '../../../src/util/log';
import { newTextEncoder } from '../../../src/platform/dom';

const ARBITRARY_SEQUENCE_NUMBER = 2;

Expand Down Expand Up @@ -456,7 +457,7 @@ abstract class TestRunner {

private async doLoadBundle(bundle: string): Promise<void> {
const reader = new BundleReader(
toByteStreamReader(new TextEncoder().encode(bundle))
toByteStreamReader(newTextEncoder().encode(bundle))
);
const task = new LoadBundleTask();
return this.queue.enqueue(async () => {
Expand Down
8 changes: 4 additions & 4 deletions packages/firestore/test/unit/util/bundle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,12 @@ import {
doc1,
doc2
} from './bundle_data';
import { newTextEncoder } from '../../../src/platform/dom';

use(chaiAsPromised);

const encoder = newTextEncoder();

/**
* Create a `ReadableStream` from a string.
*
Expand All @@ -53,14 +56,13 @@ export function byteStreamReaderFromString(
content: string,
bytesPerRead: number
): ReadableStreamReader<Uint8Array> {
const data = new TextEncoder().encode(content);
const data = encoder.encode(content);
return toByteStreamReader(data, bytesPerRead);
}

// Testing readableStreamFromString() is working as expected.
describe('byteStreamReaderFromString()', () => {
it('returns a reader stepping readable stream', async () => {
const encoder = new TextEncoder();
const r = byteStreamReaderFromString('0123456789', 4);

let result = await r.read();
Expand Down Expand Up @@ -93,8 +95,6 @@ function genericBundleReadingTests(bytesPerRead: number): void {
return new BundleReader(byteStreamReaderFromString(s, bytesPerRead));
}

const encoder = new TextEncoder();

async function getAllElements(
bundle: BundleReader
): Promise<SizedBundleElement[]> {
Expand Down
Loading