Skip to content

Remove instanceof IndexedDbRemoteDocumentCache check #2566

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 1 commit into from
Jan 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 3 additions & 20 deletions packages/firestore/src/local/indexeddb_remote_document_cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -291,11 +291,6 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
.next(() => results);
}

/**
* Returns the set of documents that have been updated since the specified read
* time.
*/
// PORTING NOTE: This is only used for multi-tab synchronization.
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive by comment: don't the comments still apply? (i.e. isn't this still only used for multi-tab?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment is already on the interface declaration.

getNewDocumentChanges(
transaction: PersistenceTransaction,
sinceReadTime: SnapshotVersion
Expand Down Expand Up @@ -328,37 +323,25 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
});
}

/**
* Returns the last document that has changed, as well as the read time of the
* last change. If no document has changed, returns SnapshotVersion.MIN.
*/
// PORTING NOTE: This is only used for multi-tab synchronization.
getLastDocumentChange(
getLastReadTime(
transaction: PersistenceTransaction
): PersistencePromise<{
changedDoc: MaybeDocument | undefined;
readTime: SnapshotVersion;
}> {
): PersistencePromise<SnapshotVersion> {
const documentsStore = remoteDocumentsStore(transaction);

// If there are no existing entries, we return SnapshotVersion.MIN.
let readTime = SnapshotVersion.MIN;
let changedDoc: MaybeDocument | undefined;

return documentsStore
.iterate(
{ index: DbRemoteDocument.readTimeIndex, reverse: true },
(key, dbRemoteDoc, control) => {
changedDoc = this.serializer.fromDbRemoteDocument(dbRemoteDoc);
if (dbRemoteDoc.readTime) {
readTime = this.serializer.fromDbTimestampKey(dbRemoteDoc.readTime);
}
control.done();
}
)
.next(() => {
return { changedDoc, readTime };
});
.next(() => readTime);
}

newChangeBuffer(options?: {
Expand Down
17 changes: 5 additions & 12 deletions packages/firestore/src/local/local_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1112,18 +1112,11 @@ export class LocalStore {
*/
// PORTING NOTE: Multi-tab only.
async synchronizeLastDocumentChangeReadTime(): Promise<void> {
if (this.remoteDocuments instanceof IndexedDbRemoteDocumentCache) {
const remoteDocumentCache = this.remoteDocuments;
return this.persistence
.runTransaction(
'Synchronize last document change read time',
'readonly-idempotent',
txn => remoteDocumentCache.getLastDocumentChange(txn)
)
.then(({ readTime }) => {
this.lastDocumentChangeReadTime = readTime;
});
}
this.lastDocumentChangeReadTime = await this.persistence.runTransaction(
'Synchronize last document change read time',
'readonly-idempotent',
txn => this.remoteDocuments.getLastReadTime(txn)
);
}
}

Expand Down
6 changes: 6 additions & 0 deletions packages/firestore/src/local/memory_remote_document_cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,12 @@ export class MemoryRemoteDocumentCache implements RemoteDocumentCache {
);
}

getLastReadTime(
transaction: PersistenceTransaction
): PersistencePromise<SnapshotVersion> {
return PersistencePromise.resolve(SnapshotVersion.MIN);
}

newChangeBuffer(options?: {
trackRemovals: boolean;
}): RemoteDocumentChangeBuffer {
Expand Down
9 changes: 9 additions & 0 deletions packages/firestore/src/local/remote_document_cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ export interface RemoteDocumentCache {
readTime: SnapshotVersion;
}>;

/**
* Returns the read time of the most recently read document in the cache, or
* SnapshotVersion.MIN if not available.
*/
// PORTING NOTE: This is only used for multi-tab synchronization.
getLastReadTime(
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why is this declaration here, whereas previously getLastDocumentChange wasn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It allows me to use this method in Persistence without checking instanceof IndexedDbRemoteDocumentCache

transaction: PersistenceTransaction
): PersistencePromise<SnapshotVersion>;

/**
* Provides access to add or update the contents of the cache. The buffer
* handles proper size accounting for the change.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ apiDescribe('Database (with internal API)', (persistence: boolean) => {
}
);
});

it('app delete leads to instance termination', async () => {
await withTestDoc(persistence, async docRef => {
await docRef.set({ foo: 'bar' });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ export class CountingQueryEngine implements QueryEngine {
});
},
getNewDocumentChanges: subject.getNewDocumentChanges,
getLastReadTime: subject.getLastReadTime,
getSize: subject.getSize,
newChangeBuffer: subject.newChangeBuffer
};
Expand Down
11 changes: 4 additions & 7 deletions packages/firestore/test/unit/local/remote_document_cache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,13 @@ describe('IndexedDbRemoteDocumentCache', () => {
await persistenceHelpers.clearTestPersistence();
});

function getLastDocumentChange(): Promise<{
changedDoc: MaybeDocument | undefined;
readTime: SnapshotVersion;
}> {
function getLastReadTime(): Promise<SnapshotVersion> {
return persistence.runTransaction(
'getLastDocumentChange',
'getLastReadTime',
'readonly-idempotent',
txn => {
const remoteDocuments = persistence.getRemoteDocumentCache();
return remoteDocuments.getLastDocumentChange(txn);
return remoteDocuments.getLastReadTime(txn);
}
);
}
Expand All @@ -115,7 +112,7 @@ describe('IndexedDbRemoteDocumentCache', () => {
dontPurgeData: true
});
cache = new TestRemoteDocumentCache(persistence);
const { readTime } = await getLastDocumentChange();
const readTime = await getLastReadTime();
const { changedDocs } = await cache.getNewDocumentChanges(readTime);
assertMatches([], changedDocs);
});
Expand Down