From fe88bbc3c4e32a80cae666ff554b078e21579c20 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 25 Jun 2020 11:10:27 -0700 Subject: [PATCH 1/8] Add getDoc() to firestore-exp --- packages/firestore/.eslintrc.js | 7 + packages/firestore/exp/index.d.ts | 2 +- packages/firestore/exp/index.node.ts | 50 ++++ packages/firestore/exp/src/api/database.ts | 108 ++++++++ packages/firestore/exp/src/api/reference.ts | 83 ++++++ packages/firestore/exp/src/api/snapshot.ts | 111 ++++++++ .../exp/test/deps/verify_dependencies.test.ts | 4 +- packages/firestore/exp/test/helpers.ts | 56 ++++ .../firestore/exp/test/integration.test.ts | 63 +++++ packages/firestore/lite/src/api/database.ts | 22 +- packages/firestore/rollup.config.exp.js | 8 +- packages/firestore/src/api/database.ts | 261 +++++++++++------- 12 files changed, 657 insertions(+), 118 deletions(-) create mode 100644 packages/firestore/exp/src/api/database.ts create mode 100644 packages/firestore/exp/src/api/reference.ts create mode 100644 packages/firestore/exp/src/api/snapshot.ts create mode 100644 packages/firestore/exp/test/helpers.ts create mode 100644 packages/firestore/exp/test/integration.test.ts diff --git a/packages/firestore/.eslintrc.js b/packages/firestore/.eslintrc.js index f89712ef662..f6bdcb1e36d 100644 --- a/packages/firestore/.eslintrc.js +++ b/packages/firestore/.eslintrc.js @@ -59,6 +59,13 @@ module.exports = { rules: { 'import/no-extraneous-dependencies': 'off' } + }, + // TODO(firestoreexp): Remove this exception when app-exp is published + { + files: ['exp/**/*.ts'], + rules: { + 'import/no-extraneous-dependencies': 'off' + } } ] }; diff --git a/packages/firestore/exp/index.d.ts b/packages/firestore/exp/index.d.ts index 78ae071fa5d..1a98baed1fd 100644 --- a/packages/firestore/exp/index.d.ts +++ b/packages/firestore/exp/index.d.ts @@ -488,6 +488,6 @@ export interface FirestoreError { declare module '@firebase/component' { interface NameServiceMapping { - 'firestore/lite': FirebaseFirestore; + 'firestore-exp': FirebaseFirestore; } } diff --git a/packages/firestore/exp/index.node.ts b/packages/firestore/exp/index.node.ts index b74f8b88d30..c5afa20b854 100644 --- a/packages/firestore/exp/index.node.ts +++ b/packages/firestore/exp/index.node.ts @@ -15,8 +15,37 @@ * limitations under the License. */ +import { version } from '../package.json'; +import { _registerComponent, registerVersion } from '@firebase/app-exp'; +import { Component, ComponentType } from '@firebase/component'; +import { Firestore } from './src/api/database'; + export { FieldPath, documentId } from '../lite/src/api/field_path'; +export { + Firestore, + initializeFirestore, + getFirestore +} from './src/api/database'; + +export { DocumentSnapshot, QueryDocumentSnapshot } from './src/api/snapshot'; + +export { SnapshotMetadata } from '../src/api/database'; + +export { + DocumentReference, + CollectionReference, + Query, + doc, + collection, + collectionGroup, + parent +} from '../lite/src/api/reference'; + +export { runTransaction, Transaction } from '../lite/src/api/transaction'; + +export { getDoc } from './src/api/reference'; + export { FieldValue, deleteField, @@ -33,3 +62,24 @@ export { Blob } from '../src/api/blob'; export { GeoPoint } from '../src/api/geo_point'; export { Timestamp } from '../src/api/timestamp'; + +export { refEqual, queryEqual } from '../lite/src/api/reference'; + +export function registerFirestore(): void { + _registerComponent( + new Component( + 'firestore-exp', + container => { + const app = container.getProvider('app-exp').getImmediate()!; + return ((app, auth) => new Firestore(app, auth))( + app, + container.getProvider('auth-internal') + ); + }, + ComponentType.PUBLIC + ) + ); + registerVersion('firestore-exp', version, 'node'); +} + +registerFirestore(); diff --git a/packages/firestore/exp/src/api/database.ts b/packages/firestore/exp/src/api/database.ts new file mode 100644 index 00000000000..998f489a443 --- /dev/null +++ b/packages/firestore/exp/src/api/database.ts @@ -0,0 +1,108 @@ +/** + * @license + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as firestore from '../../index'; + +import { _getProvider } from '@firebase/app-exp'; +import { FirebaseApp, _FirebaseService } from '@firebase/app-types-exp'; +import { Provider } from '@firebase/component'; + +import { FirebaseAuthInternalName } from '@firebase/auth-interop-types'; +import { FirestoreClient } from '../../../src/core/firestore_client'; +import { AsyncQueue } from '../../../src/util/async_queue'; +import { + ComponentProvider, + MemoryComponentProvider +} from '../../../src/core/component_provider'; + +import { Firestore as LiteFirestore } from '../../../lite/src/api/database'; + +/** + * The root reference to the Firestore database and the entry point for the + * tree-shakeable SDK. + */ +export class Firestore extends LiteFirestore + implements firestore.FirebaseFirestore, _FirebaseService { + private readonly _queue = new AsyncQueue(); + private readonly _persistenceKey: string; + private _componentProvider: ComponentProvider = new MemoryComponentProvider(); + + // Assigned via _getFirestoreClient() + private _firestoreClientPromise?: Promise; + + // We override the Settings property of the Lite SDK since the full Firestore + // SDK supports more settings. + protected _settings?: firestore.Settings; + + constructor( + app: FirebaseApp, + authProvider: Provider + ) { + super(app, authProvider); + this._persistenceKey = app.name; + } + + _getSettings(): firestore.Settings { + if (!this._settings) { + this._settings = {}; + } + return this._settings; + } + + _getFirestoreClient(): Promise { + if (!this._firestoreClientPromise) { + const settings = this._getSettings(); + const databaseInfo = this._makeDatabaseInfo( + settings.host, + settings.ssl, + settings.experimentalForceLongPolling + ); + + const firestoreClient = new FirestoreClient( + databaseInfo, + this._credentials, + this._queue + ); + + this._firestoreClientPromise = firestoreClient + .start(this._componentProvider, { durable: false }) + .then(() => firestoreClient); + } + + return this._firestoreClientPromise; + } + + async delete(): Promise { + // TODO(firestoreexp): Call terminate() once implemented + } +} + +export function initializeFirestore( + app: FirebaseApp, + settings: firestore.Settings +): Firestore { + const firestore = _getProvider( + app, + 'firestore-exp' + ).getImmediate() as Firestore; + firestore._configureClient(settings); + return firestore; +} + +export function getFirestore(app: FirebaseApp): Firestore { + return _getProvider(app, 'firestore-exp').getImmediate() as Firestore; +} diff --git a/packages/firestore/exp/src/api/reference.ts b/packages/firestore/exp/src/api/reference.ts new file mode 100644 index 00000000000..81ee253ffeb --- /dev/null +++ b/packages/firestore/exp/src/api/reference.ts @@ -0,0 +1,83 @@ +/** + * @license + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// See https://github.com/typescript-eslint/typescript-eslint/issues/363 +// eslint-disable-next-line @typescript-eslint/no-unused-vars +import * as firestore from '../../index'; + +import { Firestore } from './database'; +import { DocumentKeyReference } from '../../../src/api/user_data_reader'; +import { debugAssert } from '../../../src/util/assert'; +import { cast } from '../../../lite/src/api/util'; +import { DocumentSnapshot } from './snapshot'; +import { Rejecter, Resolver } from '../../../src/util/promise'; +import { + getDocViaSnapshotListener, + SnapshotMetadata +} from '../../../src/api/database'; +import { ViewSnapshot } from '../../../src/core/view_snapshot'; +import { DocumentReference } from '../../../lite/src/api/reference'; + +export function getDoc( + reference: firestore.DocumentReference +): Promise> { + const ref = cast>(reference, DocumentReference); + const firestore = cast(ref.firestore, Firestore); + return firestore._getFirestoreClient().then(async firestoreClient => { + return new Promise( + (resolve: Resolver>, reject: Rejecter) => { + getDocViaSnapshotListener( + firestoreClient, + ref, + async snapshot => { + const viewSnapshot = await snapshot; + resolve( + viewSnapshot + ? convertToDocSnapshot(firestore, ref, viewSnapshot) + : undefined + ); + }, + reject + ); + } + ); + }); +} + +/** + * Converts a ViewSnapshot that contains the single document specified by `ref` + * to a DocumentSnapshot. + */ +function convertToDocSnapshot( + firestore: Firestore, + ref: DocumentKeyReference, + snapshot: ViewSnapshot +): DocumentSnapshot { + debugAssert( + snapshot.docs.size <= 1, + 'Too many documents returned on a document query' + ); + const doc = snapshot.docs.get(ref._key); + + return new DocumentSnapshot( + firestore, + ref._key, + doc, + ref._converter, + new SnapshotMetadata(snapshot.hasPendingWrites, snapshot.fromCache) + ); +} diff --git a/packages/firestore/exp/src/api/snapshot.ts b/packages/firestore/exp/src/api/snapshot.ts new file mode 100644 index 00000000000..52f02ec1a8a --- /dev/null +++ b/packages/firestore/exp/src/api/snapshot.ts @@ -0,0 +1,111 @@ +/** + * @license + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as firestore from '../../index'; + +import { DocumentKey } from '../../../src/model/document_key'; +import { Document } from '../../../src/model/document'; +import { + ServerTimestampBehavior, + UserDataWriter +} from '../../../src/api/user_data_writer'; +import { + fieldPathFromArgument, + DocumentSnapshot as LiteDocumentSnapshot +} from '../../../lite/src/api/snapshot'; +import { Firestore } from './database'; +import { cast } from '../../../lite/src/api/util'; +import { DocumentReference } from '../../../lite/src/api/reference'; +import { SnapshotMetadata } from '../../../src/api/database'; + +const DEFAULT_SERVER_TIMESTAMP_BEHAVIOR: ServerTimestampBehavior = 'none'; + +export class DocumentSnapshot + extends LiteDocumentSnapshot + implements firestore.DocumentSnapshot { + private readonly _firestoreImpl: Firestore; + + constructor( + readonly _firestore: Firestore, + key: DocumentKey, + document: Document | null, + converter: firestore.FirestoreDataConverter | null, + readonly metadata: firestore.SnapshotMetadata + ) { + super(_firestore, key, document, converter); + this._firestoreImpl = cast(_firestore, Firestore); + } + + exists(): this is firestore.QueryDocumentSnapshot { + return super.exists(); + } + + data(options: firestore.SnapshotOptions = {}): T | undefined { + if (!this._document) { + return undefined; + } else if (this._converter) { + // We only want to use the converter and create a new DocumentSnapshot + // if a converter has been provided. + const snapshot = new QueryDocumentSnapshot( + this._firestore, + this._key, + this._document, + /* converter= */ null, + this.metadata + ); + return this._converter.fromFirestore(snapshot); + } else { + const userDataWriter = new UserDataWriter( + this._firestoreImpl._databaseId, + /* timestampsInSnapshots= */ true, + options.serverTimestamps || DEFAULT_SERVER_TIMESTAMP_BEHAVIOR, + key => + new DocumentReference(this._firestore, key, /* converter= */ null) + ); + return userDataWriter.convertValue(this._document.toProto()) as T; + } + } + + get( + fieldPath: string | firestore.FieldPath, + options: firestore.SnapshotOptions = {} + ): unknown { + if (this._document) { + const value = this._document + .data() + .field(fieldPathFromArgument('DocumentSnapshot.get', fieldPath)); + if (value !== null) { + const userDataWriter = new UserDataWriter( + this._firestoreImpl._databaseId, + /* timestampsInSnapshots= */ true, + options.serverTimestamps || DEFAULT_SERVER_TIMESTAMP_BEHAVIOR, + key => new DocumentReference(this._firestore, key, this._converter) + ); + return userDataWriter.convertValue(value); + } + } + return undefined; + } +} + +export class QueryDocumentSnapshot + extends DocumentSnapshot + implements firestore.QueryDocumentSnapshot { + data(options: firestore.SnapshotOptions = {}): T { + return super.data(options) as T; + } +} diff --git a/packages/firestore/exp/test/deps/verify_dependencies.test.ts b/packages/firestore/exp/test/deps/verify_dependencies.test.ts index c4b77e98c43..369e7783d76 100644 --- a/packages/firestore/exp/test/deps/verify_dependencies.test.ts +++ b/packages/firestore/exp/test/deps/verify_dependencies.test.ts @@ -20,7 +20,7 @@ import { expect } from 'chai'; import { extractDependencies } from '../../../../../scripts/exp/extract-deps.helpers'; import * as dependencies from './dependencies.json'; -import * as pkg from '../../../package.json'; +import * as pkg from '../../package.json'; import { forEach } from '../../../src/util/obj'; // TODO(firestorexp): Enable test @@ -28,7 +28,7 @@ import { forEach } from '../../../src/util/obj'; describe.skip('Dependencies', () => { forEach(dependencies, (api, { dependencies }) => { it(api, () => { - return extractDependencies(api, pkg.exp).then(extractedDependencies => { + return extractDependencies(api, pkg.main).then(extractedDependencies => { expect(extractedDependencies.classes).to.have.members( dependencies.classes, 'for classes' diff --git a/packages/firestore/exp/test/helpers.ts b/packages/firestore/exp/test/helpers.ts new file mode 100644 index 00000000000..98b9103b578 --- /dev/null +++ b/packages/firestore/exp/test/helpers.ts @@ -0,0 +1,56 @@ +/** + * @license + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { initializeApp } from '@firebase/app-exp'; + +import * as firestore from '../index'; + +import { initializeFirestore, doc } from '../index.node'; + +import { + DEFAULT_PROJECT_ID, + DEFAULT_SETTINGS +} from '../../test/integration/util/settings'; +import { collection } from '../../lite/src/api/reference'; + +let appCount = 0; + +export async function withTestDbSettings( + projectId: string, + settings: firestore.Settings, + fn: (db: firestore.FirebaseFirestore) => void | Promise +): Promise { + const app = initializeApp( + { apiKey: 'fake-api-key', projectId }, + 'test-app-' + appCount++ + ); + + const firestore = initializeFirestore(app, settings); + return fn(firestore); +} + +export function withTestDb( + fn: (db: firestore.FirebaseFirestore) => void | Promise +): Promise { + return withTestDbSettings(DEFAULT_PROJECT_ID, DEFAULT_SETTINGS, fn); +} + +export function withTestDoc( + fn: (doc: firestore.DocumentReference) => void | Promise +): Promise { + return withTestDb(db => fn(doc(collection(db, 'test-collection')))); +} diff --git a/packages/firestore/exp/test/integration.test.ts b/packages/firestore/exp/test/integration.test.ts new file mode 100644 index 00000000000..e5eadb81393 --- /dev/null +++ b/packages/firestore/exp/test/integration.test.ts @@ -0,0 +1,63 @@ +/** + * @license + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { initializeApp } from '@firebase/app-exp'; +import { expect, use } from 'chai'; +import * as chaiAsPromised from 'chai-as-promised'; + +import { + Firestore, + getFirestore, + initializeFirestore +} from '../src/api/database'; +import { withTestDoc } from './helpers'; +import { getDoc } from '../src/api/reference'; + +use(chaiAsPromised); + +describe('Firestore', () => { + it('can provide setting', () => { + const app = initializeApp( + { apiKey: 'fake-api-key', projectId: 'test-project' }, + 'test-app-initializeFirestore' + ); + const fs1 = initializeFirestore(app, { host: 'localhost', ssl: false }); + expect(fs1).to.be.an.instanceOf(Firestore); + }); + + it('returns same instance', () => { + const app = initializeApp( + { apiKey: 'fake-api-key', projectId: 'test-project' }, + 'test-app-getFirestore' + ); + const fs1 = getFirestore(app); + const fs2 = getFirestore(app); + expect(fs1 === fs2).to.be.true; + }); +}); + +describe('getDoc()', () => { + it('can get a non-existing document', () => { + return withTestDoc(async docRef => { + const docSnap = await getDoc(docRef); + expect(docSnap.metadata.fromCache).to.be.false; + expect(docSnap.metadata.hasPendingWrites).to.be.false; + expect(docSnap.data()).to.be.undefined; + expect(docSnap.exists()).to.be.false; + }); + }); +}); diff --git a/packages/firestore/lite/src/api/database.ts b/packages/firestore/lite/src/api/database.ts index b816ae445d6..76843428644 100644 --- a/packages/firestore/lite/src/api/database.ts +++ b/packages/firestore/lite/src/api/database.ts @@ -41,6 +41,7 @@ import { Settings } from '../../'; // settings() defaults: const DEFAULT_HOST = 'firestore.googleapis.com'; const DEFAULT_SSL = true; +const DEFAULT_FORCE_LONG_POLLING = false; // Used by full SDK /** * The root reference to the Firestore Lite database. @@ -49,10 +50,10 @@ export class Firestore implements firestore.FirebaseFirestore, _FirebaseService { readonly _databaseId: DatabaseId; private readonly _firebaseApp: FirebaseApp; - private readonly _credentials: CredentialsProvider; + protected readonly _credentials: CredentialsProvider; - // Assigned via _configureClient()/_ensureClientConfigured() - private _settings?: firestore.Settings; + // Assigned via _configureClient() + protected _settings?: firestore.Settings; private _datastorePromise?: Promise; constructor( @@ -89,7 +90,8 @@ export class Firestore _getDatastore(): Promise { if (!this._datastorePromise) { - const databaseInfo = this._makeDatabaseInfo(this._getSettings()); + const settings = this._getSettings(); + const databaseInfo = this._makeDatabaseInfo(settings.host, settings.ssl); this._datastorePromise = newConnection(databaseInfo).then(connection => { const serializer = newSerializer(databaseInfo.databaseId); return newDatastore(connection, this._credentials, serializer); @@ -99,13 +101,17 @@ export class Firestore return this._datastorePromise; } - private _makeDatabaseInfo(settings: firestore.Settings): DatabaseInfo { + protected _makeDatabaseInfo( + host?: string, + ssl?: boolean, + forceLongPolling?: boolean + ): DatabaseInfo { return new DatabaseInfo( this._databaseId, /* persistenceKey= */ 'unsupported', - settings.host ?? DEFAULT_HOST, - settings.ssl ?? DEFAULT_SSL, - /* forceLongPolling= */ false + host ?? DEFAULT_HOST, + ssl ?? DEFAULT_SSL, + forceLongPolling ?? DEFAULT_FORCE_LONG_POLLING ); } diff --git a/packages/firestore/rollup.config.exp.js b/packages/firestore/rollup.config.exp.js index 96fd519e86a..4038620726c 100644 --- a/packages/firestore/rollup.config.exp.js +++ b/packages/firestore/rollup.config.exp.js @@ -15,15 +15,18 @@ * limitations under the License. */ +import json from 'rollup-plugin-json'; +import alias from '@rollup/plugin-alias'; import typescriptPlugin from 'rollup-plugin-typescript2'; import typescript from 'typescript'; import path from 'path'; -import { resolveNodeExterns } from './rollup.shared'; +import { generateAliasConfig, resolveNodeExterns } from './rollup.shared'; import pkg from './exp/package.json'; const defaultPlugins = [ + alias(generateAliasConfig('node')), typescriptPlugin({ typescript, tsconfigOverride: { @@ -32,7 +35,8 @@ const defaultPlugins = [ } }, clean: true - }) + }), + json({ preferConst: true }) ]; const nodeBuilds = [ diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index cdce37dcae3..a0ac7ded1dc 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -1130,7 +1130,7 @@ export class DocumentReference let options: firestore.SnapshotListenOptions = { includeMetadataChanges: false }; - let observer: PartialObserver>; + let observer: PartialObserver; let currArg = 0; if ( typeof args[currArg] === 'object' && @@ -1154,9 +1154,18 @@ export class DocumentReference }; if (isPartialObserver(args[currArg])) { - observer = args[currArg] as PartialObserver< + const userObserver = args[currArg] as PartialObserver< firestore.DocumentSnapshot >; + observer = { + next: snapshot => { + if (userObserver.next) { + userObserver.next(this._convertToDocSnapshot(snapshot)); + } + }, + error: userObserver.error, + complete: userObserver.complete + }; } else { validateArgType( 'DocumentReference.onSnapshot', @@ -1177,58 +1186,23 @@ export class DocumentReference args[currArg + 2] ); observer = { - next: args[currArg] as NextFn>, + next: snapshot => { + if (args[currArg]) { + (args[currArg] as NextFn>)( + this._convertToDocSnapshot(snapshot) + ); + } + }, error: args[currArg + 1] as ErrorFn, complete: args[currArg + 2] as CompleteFn }; } - return this.onSnapshotInternal(internalOptions, observer); - } - - private onSnapshotInternal( - options: ListenOptions, - observer: PartialObserver> - ): Unsubscribe { - let errHandler = (err: Error): void => { - console.error('Uncaught Error in onSnapshot:', err); - }; - if (observer.error) { - errHandler = observer.error.bind(observer); - } - - const asyncObserver = new AsyncObserver({ - next: snapshot => { - if (observer.next) { - debugAssert( - snapshot.docs.size <= 1, - 'Too many documents returned on a document query' - ); - const doc = snapshot.docs.get(this._key); - - observer.next( - new DocumentSnapshot( - this.firestore, - this._key, - doc, - snapshot.fromCache, - snapshot.hasPendingWrites, - this._converter - ) - ); - } - }, - error: errHandler - }); - const internalListener = this._firestoreClient.listen( - InternalQuery.atPath(this._key.path), - asyncObserver, - options + return addDocSnapshotListener( + this._firestoreClient, + this, + internalOptions, + observer ); - - return () => { - asyncObserver.mute(); - this._firestoreClient.unlisten(internalListener); - }; } get(options?: firestore.GetOptions): Promise> { @@ -1253,74 +1227,151 @@ export class DocumentReference ); }, reject); } else { - this.getViaSnapshotListener(resolve, reject, options); + getDocViaSnapshotListener( + this._firestoreClient, + this, + async snapshot => { + const viewSnapshot = await snapshot; + resolve( + viewSnapshot + ? this._convertToDocSnapshot(viewSnapshot) + : undefined + ); + }, + reject, + options + ); } } ); } - private getViaSnapshotListener( - resolve: Resolver>, - reject: Rejecter, - options?: firestore.GetOptions - ): void { - const unlisten = this.onSnapshotInternal( - { - includeMetadataChanges: true, - waitForSyncWhenOnline: true - }, - { - next: (snap: firestore.DocumentSnapshot) => { - // Remove query first before passing event to user to avoid - // user actions affecting the now stale query. - unlisten(); - - if (!snap.exists && snap.metadata.fromCache) { - // TODO(dimond): If we're online and the document doesn't - // exist then we resolve with a doc.exists set to false. If - // we're offline however, we reject the Promise in this - // case. Two options: 1) Cache the negative response from - // the server so we can deliver that even when you're - // offline 2) Actually reject the Promise in the online case - // if the document doesn't exist. - reject( - new FirestoreError( - Code.UNAVAILABLE, - 'Failed to get document because the client is ' + 'offline.' - ) - ); - } else if ( - snap.exists && - snap.metadata.fromCache && - options && - options.source === 'server' - ) { - reject( - new FirestoreError( - Code.UNAVAILABLE, - 'Failed to get document from server. (However, this ' + - 'document does exist in the local cache. Run again ' + - 'without setting source to "server" to ' + - 'retrieve the cached document.)' - ) - ); - } else { - resolve(snap); - } - }, - error: reject - } - ); - } - withConverter( converter: firestore.FirestoreDataConverter ): firestore.DocumentReference { return new DocumentReference(this._key, this.firestore, converter); } + + /** + * Converts a ViewSnapshot that contains the current document to a + * DocumentSnapshot. + */ + private _convertToDocSnapshot(snapshot: ViewSnapshot): DocumentSnapshot { + debugAssert( + snapshot.docs.size <= 1, + 'Too many documents returned on a document query' + ); + const doc = snapshot.docs.get(this._key); + + return new DocumentSnapshot( + this.firestore, + this._key, + doc, + snapshot.fromCache, + snapshot.hasPendingWrites, + this._converter + ); + } +} + +/** Registers an internal snapshot listener for `ref`. */ +function addDocSnapshotListener( + firestoreClient: FirestoreClient, + ref: DocumentKeyReference, + options: ListenOptions, + observer: PartialObserver +): Unsubscribe { + let errHandler = (err: Error): void => { + console.error('Uncaught Error in onSnapshot:', err); + }; + if (observer.error) { + errHandler = observer.error.bind(observer); + } + + const asyncObserver = new AsyncObserver({ + next: snapshot => { + if (observer.next) { + observer.next(snapshot); + } + }, + error: errHandler + }); + const internalListener = firestoreClient.listen( + InternalQuery.atPath(ref._key.path), + asyncObserver, + options + ); + + return () => { + asyncObserver.mute(); + firestoreClient.unlisten(internalListener); + }; +} + +/** + * Retrieves a latency-compensated document from the backend via a + * SnapshotListener. + */ +export function getDocViaSnapshotListener( + firestoreClient: FirestoreClient, + ref: DocumentKeyReference, + resolve: Resolver, + reject: Rejecter, + options?: firestore.GetOptions +): void { + const unlisten = addDocSnapshotListener( + firestoreClient, + ref, + { + includeMetadataChanges: true, + waitForSyncWhenOnline: true + }, + { + next: (snap: ViewSnapshot) => { + // Remove query first before passing event to user to avoid + // user actions affecting the now stale query. + unlisten(); + + const exists = snap.docs.has(ref._key); + if (!exists && snap.fromCache) { + // TODO(dimond): If we're online and the document doesn't + // exist then we resolve with a doc.exists set to false. If + // we're offline however, we reject the Promise in this + // case. Two options: 1) Cache the negative response from + // the server so we can deliver that even when you're + // offline 2) Actually reject the Promise in the online case + // if the document doesn't exist. + reject( + new FirestoreError( + Code.UNAVAILABLE, + 'Failed to get document because the client is ' + 'offline.' + ) + ); + } else if ( + exists && + snap.fromCache && + options && + options.source === 'server' + ) { + reject( + new FirestoreError( + Code.UNAVAILABLE, + 'Failed to get document from server. (However, this ' + + 'document does exist in the local cache. Run again ' + + 'without setting source to "server" to ' + + 'retrieve the cached document.)' + ) + ); + } else { + resolve(snap); + } + }, + error: reject + } + ); } -class SnapshotMetadata implements firestore.SnapshotMetadata { +export class SnapshotMetadata implements firestore.SnapshotMetadata { constructor( readonly hasPendingWrites: boolean, readonly fromCache: boolean From 64162a287bda23971621a7d45a330966ab3d7927 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 25 Jun 2020 11:45:02 -0700 Subject: [PATCH 2/8] Simplify --- packages/firestore/exp/src/api/reference.ts | 20 +----- packages/firestore/src/api/database.ts | 73 +++++++++------------ 2 files changed, 32 insertions(+), 61 deletions(-) diff --git a/packages/firestore/exp/src/api/reference.ts b/packages/firestore/exp/src/api/reference.ts index 81ee253ffeb..4328ca88d76 100644 --- a/packages/firestore/exp/src/api/reference.ts +++ b/packages/firestore/exp/src/api/reference.ts @@ -24,7 +24,6 @@ import { DocumentKeyReference } from '../../../src/api/user_data_reader'; import { debugAssert } from '../../../src/util/assert'; import { cast } from '../../../lite/src/api/util'; import { DocumentSnapshot } from './snapshot'; -import { Rejecter, Resolver } from '../../../src/util/promise'; import { getDocViaSnapshotListener, SnapshotMetadata @@ -38,23 +37,8 @@ export function getDoc( const ref = cast>(reference, DocumentReference); const firestore = cast(ref.firestore, Firestore); return firestore._getFirestoreClient().then(async firestoreClient => { - return new Promise( - (resolve: Resolver>, reject: Rejecter) => { - getDocViaSnapshotListener( - firestoreClient, - ref, - async snapshot => { - const viewSnapshot = await snapshot; - resolve( - viewSnapshot - ? convertToDocSnapshot(firestore, ref, viewSnapshot) - : undefined - ); - }, - reject - ); - } - ); + const viewSnapshot = await getDocViaSnapshotListener(firestoreClient, ref); + return convertToDocSnapshot(firestore, ref, viewSnapshot); }); } diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index a0ac7ded1dc..04aa40daf90 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -1208,42 +1208,29 @@ export class DocumentReference get(options?: firestore.GetOptions): Promise> { validateBetweenNumberOfArgs('DocumentReference.get', arguments, 0, 1); validateGetOptions('DocumentReference.get', options); - return new Promise( - (resolve: Resolver>, reject: Rejecter) => { - if (options && options.source === 'cache') { - this.firestore - .ensureClientConfigured() - .getDocumentFromLocalCache(this._key) - .then(doc => { - resolve( - new DocumentSnapshot( - this.firestore, - this._key, - doc, - /*fromCache=*/ true, - doc instanceof Document ? doc.hasLocalMutations : false, - this._converter - ) - ); - }, reject); - } else { - getDocViaSnapshotListener( - this._firestoreClient, - this, - async snapshot => { - const viewSnapshot = await snapshot; - resolve( - viewSnapshot - ? this._convertToDocSnapshot(viewSnapshot) - : undefined - ); - }, - reject, - options - ); - } - } - ); + + if (options && options.source === 'cache') { + return this.firestore + .ensureClientConfigured() + .getDocumentFromLocalCache(this._key) + .then( + doc => + new DocumentSnapshot( + this.firestore, + this._key, + doc, + /*fromCache=*/ true, + doc instanceof Document ? doc.hasLocalMutations : false, + this._converter + ) + ); + } else { + return getDocViaSnapshotListener( + this._firestoreClient, + this, + options + ).then(snapshot => this._convertToDocSnapshot(snapshot)); + } } withConverter( @@ -1315,10 +1302,9 @@ function addDocSnapshotListener( export function getDocViaSnapshotListener( firestoreClient: FirestoreClient, ref: DocumentKeyReference, - resolve: Resolver, - reject: Rejecter, options?: firestore.GetOptions -): void { +): Promise { + const result = new Deferred(); const unlisten = addDocSnapshotListener( firestoreClient, ref, @@ -1341,7 +1327,7 @@ export function getDocViaSnapshotListener( // the server so we can deliver that even when you're // offline 2) Actually reject the Promise in the online case // if the document doesn't exist. - reject( + result.reject( new FirestoreError( Code.UNAVAILABLE, 'Failed to get document because the client is ' + 'offline.' @@ -1353,7 +1339,7 @@ export function getDocViaSnapshotListener( options && options.source === 'server' ) { - reject( + result.reject( new FirestoreError( Code.UNAVAILABLE, 'Failed to get document from server. (However, this ' + @@ -1363,12 +1349,13 @@ export function getDocViaSnapshotListener( ) ); } else { - resolve(snap); + result.resolve(snap); } }, - error: reject + error: e => result.reject(e) } ); + return result.promise; } export class SnapshotMetadata implements firestore.SnapshotMetadata { From 9cf0d8474cd93ccf837295f53ede79babeaadd52 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 26 Jun 2020 12:56:04 -0700 Subject: [PATCH 3/8] Review --- packages/firestore/exp/src/api/reference.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/exp/src/api/reference.ts b/packages/firestore/exp/src/api/reference.ts index 4328ca88d76..e8125dd38a8 100644 --- a/packages/firestore/exp/src/api/reference.ts +++ b/packages/firestore/exp/src/api/reference.ts @@ -53,7 +53,7 @@ function convertToDocSnapshot( ): DocumentSnapshot { debugAssert( snapshot.docs.size <= 1, - 'Too many documents returned on a document query' + 'Too many documents returned on a single document query' ); const doc = snapshot.docs.get(ref._key); From b09863f8534902e1b35823be35b18c5ba5542ba6 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 26 Jun 2020 12:59:00 -0700 Subject: [PATCH 4/8] Better error --- packages/firestore/exp/src/api/reference.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/exp/src/api/reference.ts b/packages/firestore/exp/src/api/reference.ts index e8125dd38a8..d543ed75034 100644 --- a/packages/firestore/exp/src/api/reference.ts +++ b/packages/firestore/exp/src/api/reference.ts @@ -53,7 +53,7 @@ function convertToDocSnapshot( ): DocumentSnapshot { debugAssert( snapshot.docs.size <= 1, - 'Too many documents returned on a single document query' + 'Expected zero or a single result on a document-only query' ); const doc = snapshot.docs.get(ref._key); From fa37773485ded57e90798052fddf2397b980e6d6 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 26 Jun 2020 13:21:29 -0700 Subject: [PATCH 5/8] Create popular-beds-yell.md --- .changeset/popular-beds-yell.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 .changeset/popular-beds-yell.md diff --git a/.changeset/popular-beds-yell.md b/.changeset/popular-beds-yell.md new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/.changeset/popular-beds-yell.md @@ -0,0 +1 @@ + From 4188756be267c918321ad84e063793fc599cc228 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 26 Jun 2020 13:30:04 -0700 Subject: [PATCH 6/8] Create popular-beds-yell2.md --- .changeset/popular-beds-yell2.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 .changeset/popular-beds-yell2.md diff --git a/.changeset/popular-beds-yell2.md b/.changeset/popular-beds-yell2.md new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/.changeset/popular-beds-yell2.md @@ -0,0 +1 @@ + From 2cf802a562534de36c110bcbfa10e39adc19e2ca Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 26 Jun 2020 13:33:54 -0700 Subject: [PATCH 7/8] Update popular-beds-yell.md --- .changeset/popular-beds-yell.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/popular-beds-yell.md b/.changeset/popular-beds-yell.md index 8b137891791..017b123feec 100644 --- a/.changeset/popular-beds-yell.md +++ b/.changeset/popular-beds-yell.md @@ -1 +1,3 @@ +--- +--- From 11bc080b1ca90867c44bc33dcf4a9eb1a7f4dc03 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 26 Jun 2020 13:34:15 -0700 Subject: [PATCH 8/8] Update popular-beds-yell2.md --- .changeset/popular-beds-yell2.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/popular-beds-yell2.md b/.changeset/popular-beds-yell2.md index 8b137891791..017b123feec 100644 --- a/.changeset/popular-beds-yell2.md +++ b/.changeset/popular-beds-yell2.md @@ -1 +1,3 @@ +--- +---