From c06b2b7728c6f97f34134df12370cde09db0e6cd Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 12 Jan 2021 16:55:28 -0500 Subject: [PATCH 01/12] Fix; needs cleanup --- packages/firestore/src/api/database.ts | 33 ++++++++++----- .../test/integration/api/database.test.ts | 40 +++++++++++++++++++ 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 526cbf6ff0a..3464c37ad43 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -99,6 +99,7 @@ import { import { DocumentChange as ExpDocumentChange, DocumentSnapshot as ExpDocumentSnapshot, + QueryDocumentSnapshot as ExpQueryDocumentSnapshot, QuerySnapshot as ExpQuerySnapshot, snapshotEqual, SnapshotMetadata @@ -552,6 +553,26 @@ export class WriteBatch } } +class PublicFirestoreDataConverterAdapter implements UntypedFirestoreDataConverter { + constructor(readonly firestore: Firestore, readonly delegate: PublicFirestoreDataConverter) {} + + fromFirestore(snapshot: unknown, options?: unknown): U { + const expSnapshot = snapshot as ExpQueryDocumentSnapshot; + const querySnapshot = new QueryDocumentSnapshot(this.firestore, expSnapshot); + return this.delegate.fromFirestore(querySnapshot, options as PublicSnapshotOptions); + } + + toFirestore(modelObject: U): PublicDocumentData; + toFirestore(modelObject: Partial, options: PublicSetOptions): PublicDocumentData; + toFirestore(modelObject: U | Partial, options?: PublicSetOptions): PublicDocumentData { + if (options === undefined) { + return this.delegate.toFirestore(modelObject as U); + } else { + return this.delegate.toFirestore(modelObject, options); + } + } +} + /** * A reference to a particular document in a collection in the database. */ @@ -752,9 +773,7 @@ export class DocumentReference ): PublicDocumentReference { return new DocumentReference( this.firestore, - this._delegate.withConverter( - converter as UntypedFirestoreDataConverter - ) + this._delegate.withConverter(new PublicFirestoreDataConverterAdapter(this.firestore, converter)), ); } } @@ -1061,9 +1080,7 @@ export class Query withConverter(converter: PublicFirestoreDataConverter): Query { return new Query( this.firestore, - this._delegate.withConverter( - converter as UntypedFirestoreDataConverter - ) + this._delegate.withConverter(new PublicFirestoreDataConverterAdapter(this.firestore, converter)), ); } } @@ -1205,9 +1222,7 @@ export class CollectionReference ): CollectionReference { return new CollectionReference( this.firestore, - this._delegate.withConverter( - converter as UntypedFirestoreDataConverter - ) + this._delegate.withConverter(new PublicFirestoreDataConverterAdapter(this.firestore, converter)), ); } } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 61cd4ca5008..e6d75ff8503 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1590,6 +1590,46 @@ apiDescribe('Database', (persistence: boolean) => { expect(docRef.isEqual(docRef2)).to.be.false; }); }); + + it('https://github.com/firebase/firebase-js-sdk/issues/4278', () => { + return withTestDb(persistence, async db => { + class MyModel { + constructor(readonly ref: firestore.DocumentReference) {} + } + + const omitSingle = (key: any, { [key]: _, ...obj }) => obj; + + const modelConverter = { + toFirestore: function (model: MyModel) { + return omitSingle("ref", model); + }, + fromFirestore: function ( + snapshot: firestore.QueryDocumentSnapshot + ): MyModel { + return new MyModel(snapshot.ref); + }, + denver: function() { + console.log("denver"); + } + }; + + const docRef = db + .collection("/models") + .doc("some_id") + .withConverter(modelConverter); + + await docRef.set(new MyModel(docRef)); + + const accumulator = new EventsAccumulator>(); + const unsubscribe = docRef.onSnapshot(accumulator.storeEvent); + + await accumulator + .awaitEvent() + .then(docSnapshot => docSnapshot.data()!.ref.collection("/sub")); + + unsubscribe(); + }); + }); }); it('can set and get data with auto detect long polling enabled', () => { From c85b8d016f53e1ba9bade88bd23c4b6875656260 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 12 Jan 2021 23:24:10 -0500 Subject: [PATCH 02/12] Format code --- packages/firestore/src/api/database.ts | 40 ++++++++++++++----- .../test/integration/api/database.test.ts | 16 ++++---- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 3464c37ad43..91fe21f7ad5 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -553,18 +553,34 @@ export class WriteBatch } } -class PublicFirestoreDataConverterAdapter implements UntypedFirestoreDataConverter { - constructor(readonly firestore: Firestore, readonly delegate: PublicFirestoreDataConverter) {} +class PublicFirestoreDataConverterAdapter + implements UntypedFirestoreDataConverter { + constructor( + readonly firestore: Firestore, + readonly delegate: PublicFirestoreDataConverter + ) {} fromFirestore(snapshot: unknown, options?: unknown): U { const expSnapshot = snapshot as ExpQueryDocumentSnapshot; - const querySnapshot = new QueryDocumentSnapshot(this.firestore, expSnapshot); - return this.delegate.fromFirestore(querySnapshot, options as PublicSnapshotOptions); + const querySnapshot = new QueryDocumentSnapshot( + this.firestore, + expSnapshot + ); + return this.delegate.fromFirestore( + querySnapshot, + options as PublicSnapshotOptions + ); } toFirestore(modelObject: U): PublicDocumentData; - toFirestore(modelObject: Partial, options: PublicSetOptions): PublicDocumentData; - toFirestore(modelObject: U | Partial, options?: PublicSetOptions): PublicDocumentData { + toFirestore( + modelObject: Partial, + options: PublicSetOptions + ): PublicDocumentData; + toFirestore( + modelObject: U | Partial, + options?: PublicSetOptions + ): PublicDocumentData { if (options === undefined) { return this.delegate.toFirestore(modelObject as U); } else { @@ -773,7 +789,9 @@ export class DocumentReference ): PublicDocumentReference { return new DocumentReference( this.firestore, - this._delegate.withConverter(new PublicFirestoreDataConverterAdapter(this.firestore, converter)), + this._delegate.withConverter( + new PublicFirestoreDataConverterAdapter(this.firestore, converter) + ) ); } } @@ -1080,7 +1098,9 @@ export class Query withConverter(converter: PublicFirestoreDataConverter): Query { return new Query( this.firestore, - this._delegate.withConverter(new PublicFirestoreDataConverterAdapter(this.firestore, converter)), + this._delegate.withConverter( + new PublicFirestoreDataConverterAdapter(this.firestore, converter) + ) ); } } @@ -1222,7 +1242,9 @@ export class CollectionReference ): CollectionReference { return new CollectionReference( this.firestore, - this._delegate.withConverter(new PublicFirestoreDataConverterAdapter(this.firestore, converter)), + this._delegate.withConverter( + new PublicFirestoreDataConverterAdapter(this.firestore, converter) + ) ); } } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index e6d75ff8503..52a5b9ce3c0 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1601,31 +1601,33 @@ apiDescribe('Database', (persistence: boolean) => { const modelConverter = { toFirestore: function (model: MyModel) { - return omitSingle("ref", model); + return omitSingle('ref', model); }, fromFirestore: function ( snapshot: firestore.QueryDocumentSnapshot ): MyModel { return new MyModel(snapshot.ref); }, - denver: function() { - console.log("denver"); + denver: function () { + console.log('denver'); } }; const docRef = db - .collection("/models") - .doc("some_id") + .collection('/models') + .doc('some_id') .withConverter(modelConverter); await docRef.set(new MyModel(docRef)); - const accumulator = new EventsAccumulator>(); + const accumulator = new EventsAccumulator< + firestore.DocumentSnapshot + >(); const unsubscribe = docRef.onSnapshot(accumulator.storeEvent); await accumulator .awaitEvent() - .then(docSnapshot => docSnapshot.data()!.ref.collection("/sub")); + .then(docSnapshot => docSnapshot.data()!.ref.collection('/sub')); unsubscribe(); }); From cf6be2a813ccd95f7f79c9aff14beb7ac7036414 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 13 Jan 2021 11:15:42 -0500 Subject: [PATCH 03/12] More work (incomplete) --- packages/firestore/src/api/database.ts | 11 ++- .../test/integration/api/database.test.ts | 84 +++++++++++-------- 2 files changed, 53 insertions(+), 42 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 91fe21f7ad5..c5f17a72ca4 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -553,7 +553,7 @@ export class WriteBatch } } -class PublicFirestoreDataConverterAdapter +class UntypedFirestoreDataConverterAdapter implements UntypedFirestoreDataConverter { constructor( readonly firestore: Firestore, @@ -561,10 +561,9 @@ class PublicFirestoreDataConverterAdapter ) {} fromFirestore(snapshot: unknown, options?: unknown): U { - const expSnapshot = snapshot as ExpQueryDocumentSnapshot; const querySnapshot = new QueryDocumentSnapshot( this.firestore, - expSnapshot + snapshot as ExpQueryDocumentSnapshot ); return this.delegate.fromFirestore( querySnapshot, @@ -790,7 +789,7 @@ export class DocumentReference return new DocumentReference( this.firestore, this._delegate.withConverter( - new PublicFirestoreDataConverterAdapter(this.firestore, converter) + new UntypedFirestoreDataConverterAdapter(this.firestore, converter) ) ); } @@ -1099,7 +1098,7 @@ export class Query return new Query( this.firestore, this._delegate.withConverter( - new PublicFirestoreDataConverterAdapter(this.firestore, converter) + new UntypedFirestoreDataConverterAdapter(this.firestore, converter) ) ); } @@ -1243,7 +1242,7 @@ export class CollectionReference return new CollectionReference( this.firestore, this._delegate.withConverter( - new PublicFirestoreDataConverterAdapter(this.firestore, converter) + new UntypedFirestoreDataConverterAdapter(this.firestore, converter) ) ); } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 52a5b9ce3c0..f2dffc888c9 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1373,6 +1373,27 @@ apiDescribe('Database', (persistence: boolean) => { } }; + class ModelWithRef { + constructor(readonly ref: firestore.DocumentReference) {} + } + + const omitSingle = (key: any, { [key]: _, ...obj }) => obj; + + const modelWithRefConverter = { + toFirestore(model: ModelWithRef): firestore.DocumentData { + return omitSingle('ref', model); + }, + fromFirestore( + snapshot: firestore.QueryDocumentSnapshot, + options: firestore.SnapshotOptions + ): ModelWithRef { + return new ModelWithRef(snapshot.ref); + }, + denver: function () { + console.log('denver'); + } + }; + it('for DocumentReference.withConverter()', () => { return withTestDb(persistence, async db => { const docRef = db @@ -1591,45 +1612,36 @@ apiDescribe('Database', (persistence: boolean) => { }); }); - it('https://github.com/firebase/firebase-js-sdk/issues/4278', () => { + it('github Correct snapshot specified to fromFirestore() when registered with DocumentReference', () => { return withTestDb(persistence, async db => { - class MyModel { - constructor(readonly ref: firestore.DocumentReference) {} - } - - const omitSingle = (key: any, { [key]: _, ...obj }) => obj; - - const modelConverter = { - toFirestore: function (model: MyModel) { - return omitSingle('ref', model); - }, - fromFirestore: function ( - snapshot: firestore.QueryDocumentSnapshot - ): MyModel { - return new MyModel(snapshot.ref); - }, - denver: function () { - console.log('denver'); - } - }; - - const docRef = db - .collection('/models') - .doc('some_id') - .withConverter(modelConverter); - - await docRef.set(new MyModel(docRef)); - - const accumulator = new EventsAccumulator< - firestore.DocumentSnapshot - >(); - const unsubscribe = docRef.onSnapshot(accumulator.storeEvent); + const docRef = db.collection('/models').doc().withConverter(modelWithRefConverter); + await docRef.set(new ModelWithRef(docRef)); + const docSnapshot = await docRef.get(); + expect(docRef.isEqual(docSnapshot.data()!.ref)).to.be.true; + }); + }); - await accumulator - .awaitEvent() - .then(docSnapshot => docSnapshot.data()!.ref.collection('/sub')); + it('github Correct snapshot specified to fromFirestore() when registered with CollectionReference', () => { + return withTestDb(persistence, async db => { + const collection = db.collection('/models').doc().collection("sub").withConverter(modelWithRefConverter); + const docRef = collection.doc(); + await docRef.set(new ModelWithRef(docRef)); + const querySnapshot = await collection.get(); + expect(querySnapshot.size).to.equal(1); + expect(docRef.isEqual(querySnapshot.docs[0].data().ref)).to.be.true; + }); + }); - unsubscribe(); + it('github Correct snapshot specified to fromFirestore() when registered with Query', () => { + return withTestDb(persistence, async db => { + const collection = db.collection('/models'); + const docRef = collection.doc(); + const docRefWithConverter = docRef.withConverter(modelWithRefConverter); + await docRefWithConverter.set(new ModelWithRef(docRefWithConverter)); + const query = collection.where(FieldPath.documentId(), '==', docRef.id).withConverter(modelWithRefConverter); + const querySnapshot = await query.get(); + expect(querySnapshot.size).to.equal(1); + expect(docRef.isEqual(querySnapshot.docs[0].data().ref)).to.be.true; }); }); }); From 376e9ec4b7967b8c3bccc890af12626243ef7542 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 14 Jan 2021 14:03:19 -0500 Subject: [PATCH 04/12] Work done? --- packages/firestore/src/api/database.ts | 62 +++++++++---------- .../test/integration/api/database.test.ts | 32 +++++----- 2 files changed, 45 insertions(+), 49 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index c5f17a72ca4..360e4980055 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -554,21 +554,20 @@ export class WriteBatch } class UntypedFirestoreDataConverterAdapter + extends Compat> implements UntypedFirestoreDataConverter { - constructor( - readonly firestore: Firestore, - readonly delegate: PublicFirestoreDataConverter - ) {} + + private static readonly INSTANCES = new WeakMap(); + + private constructor( + private readonly firestore: Firestore, + delegate: PublicFirestoreDataConverter + ) { + super(delegate); + } fromFirestore(snapshot: unknown, options?: unknown): U { - const querySnapshot = new QueryDocumentSnapshot( - this.firestore, - snapshot as ExpQueryDocumentSnapshot - ); - return this.delegate.fromFirestore( - querySnapshot, - options as PublicSnapshotOptions - ); + return this._delegate.fromFirestore(new QueryDocumentSnapshot(this.firestore, snapshot as ExpQueryDocumentSnapshot), options as PublicSnapshotOptions); } toFirestore(modelObject: U): PublicDocumentData; @@ -580,14 +579,26 @@ class UntypedFirestoreDataConverterAdapter modelObject: U | Partial, options?: PublicSetOptions ): PublicDocumentData { - if (options === undefined) { - return this.delegate.toFirestore(modelObject as U); + if (!options) { + return this._delegate.toFirestore(modelObject as U); } else { - return this.delegate.toFirestore(modelObject, options); + return this._delegate.toFirestore(modelObject, options); + } + } + + static getInstance(firestore: Firestore, converter: PublicFirestoreDataConverter): UntypedFirestoreDataConverterAdapter { + const map = UntypedFirestoreDataConverterAdapter.INSTANCES; + let instance = map.get(converter); + if (! instance) { + instance = new UntypedFirestoreDataConverterAdapter(firestore, converter); + map.set(converter, instance); } + return instance; } } + + /** * A reference to a particular document in a collection in the database. */ @@ -786,12 +797,7 @@ export class DocumentReference withConverter( converter: PublicFirestoreDataConverter ): PublicDocumentReference { - return new DocumentReference( - this.firestore, - this._delegate.withConverter( - new UntypedFirestoreDataConverterAdapter(this.firestore, converter) - ) - ); + return new DocumentReference(this.firestore, this._delegate.withConverter(UntypedFirestoreDataConverterAdapter.getInstance(this.firestore, converter))); } } @@ -1095,12 +1101,7 @@ export class Query } withConverter(converter: PublicFirestoreDataConverter): Query { - return new Query( - this.firestore, - this._delegate.withConverter( - new UntypedFirestoreDataConverterAdapter(this.firestore, converter) - ) - ); + return new Query(this.firestore, this._delegate.withConverter(UntypedFirestoreDataConverterAdapter.getInstance(this.firestore, converter))); } } @@ -1239,12 +1240,7 @@ export class CollectionReference withConverter( converter: PublicFirestoreDataConverter ): CollectionReference { - return new CollectionReference( - this.firestore, - this._delegate.withConverter( - new UntypedFirestoreDataConverterAdapter(this.firestore, converter) - ) - ); + return new CollectionReference(this.firestore, this._delegate.withConverter(UntypedFirestoreDataConverterAdapter.getInstance(this.firestore, converter))); } } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index f2dffc888c9..cf672fc4231 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1389,9 +1389,6 @@ apiDescribe('Database', (persistence: boolean) => { ): ModelWithRef { return new ModelWithRef(snapshot.ref); }, - denver: function () { - console.log('denver'); - } }; it('for DocumentReference.withConverter()', () => { @@ -1612,36 +1609,39 @@ apiDescribe('Database', (persistence: boolean) => { }); }); - it('github Correct snapshot specified to fromFirestore() when registered with DocumentReference', () => { + it('Correct snapshot specified to fromFirestore() when registered with DocumentReference', () => { return withTestDb(persistence, async db => { - const docRef = db.collection('/models').doc().withConverter(modelWithRefConverter); + const untypedDocRef = db.collection('/models').doc(); + const docRef = untypedDocRef.withConverter(modelWithRefConverter); await docRef.set(new ModelWithRef(docRef)); const docSnapshot = await docRef.get(); - expect(docRef.isEqual(docSnapshot.data()!.ref)).to.be.true; + expect(untypedDocRef.isEqual(docSnapshot.data()!.ref)).to.be.true; }); }); - it('github Correct snapshot specified to fromFirestore() when registered with CollectionReference', () => { + it('Correct snapshot specified to fromFirestore() when registered with CollectionReference', () => { return withTestDb(persistence, async db => { - const collection = db.collection('/models').doc().collection("sub").withConverter(modelWithRefConverter); + const untypedCollection = db.collection('/models').doc().collection("sub"); + const collection = untypedCollection.withConverter(modelWithRefConverter); const docRef = collection.doc(); await docRef.set(new ModelWithRef(docRef)); const querySnapshot = await collection.get(); expect(querySnapshot.size).to.equal(1); - expect(docRef.isEqual(querySnapshot.docs[0].data().ref)).to.be.true; + const untypedDocRef = untypedCollection.doc(docRef.id); + expect(untypedDocRef.isEqual(querySnapshot.docs[0].data().ref)).to.be.true; }); }); - it('github Correct snapshot specified to fromFirestore() when registered with Query', () => { + it('Correct snapshot specified to fromFirestore() when registered with Query', () => { return withTestDb(persistence, async db => { - const collection = db.collection('/models'); - const docRef = collection.doc(); - const docRefWithConverter = docRef.withConverter(modelWithRefConverter); - await docRefWithConverter.set(new ModelWithRef(docRefWithConverter)); - const query = collection.where(FieldPath.documentId(), '==', docRef.id).withConverter(modelWithRefConverter); + const untypedCollection = db.collection('/models'); + const untypedDocRef = untypedCollection.doc(); + const docRef = untypedDocRef.withConverter(modelWithRefConverter); + await docRef.set(new ModelWithRef(docRef)); + const query = untypedCollection.where(FieldPath.documentId(), '==', docRef.id).withConverter(modelWithRefConverter); const querySnapshot = await query.get(); expect(querySnapshot.size).to.equal(1); - expect(docRef.isEqual(querySnapshot.docs[0].data().ref)).to.be.true; + expect(untypedDocRef.isEqual(querySnapshot.docs[0].data().ref)).to.be.true; }); }); }); From e6cd06b456216d4ef8b2a5ad676e2f31a89a74a5 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 14 Jan 2021 14:05:10 -0500 Subject: [PATCH 05/12] Format code --- packages/firestore/src/api/database.ts | 48 +++++++++++++++---- .../test/integration/api/database.test.ts | 21 +++++--- 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 360e4980055..068f8814473 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -556,7 +556,6 @@ export class WriteBatch class UntypedFirestoreDataConverterAdapter extends Compat> implements UntypedFirestoreDataConverter { - private static readonly INSTANCES = new WeakMap(); private constructor( @@ -567,7 +566,13 @@ class UntypedFirestoreDataConverterAdapter } fromFirestore(snapshot: unknown, options?: unknown): U { - return this._delegate.fromFirestore(new QueryDocumentSnapshot(this.firestore, snapshot as ExpQueryDocumentSnapshot), options as PublicSnapshotOptions); + return this._delegate.fromFirestore( + new QueryDocumentSnapshot( + this.firestore, + snapshot as ExpQueryDocumentSnapshot + ), + options as PublicSnapshotOptions + ); } toFirestore(modelObject: U): PublicDocumentData; @@ -586,10 +591,13 @@ class UntypedFirestoreDataConverterAdapter } } - static getInstance(firestore: Firestore, converter: PublicFirestoreDataConverter): UntypedFirestoreDataConverterAdapter { + static getInstance( + firestore: Firestore, + converter: PublicFirestoreDataConverter + ): UntypedFirestoreDataConverterAdapter { const map = UntypedFirestoreDataConverterAdapter.INSTANCES; let instance = map.get(converter); - if (! instance) { + if (!instance) { instance = new UntypedFirestoreDataConverterAdapter(firestore, converter); map.set(converter, instance); } @@ -597,8 +605,6 @@ class UntypedFirestoreDataConverterAdapter } } - - /** * A reference to a particular document in a collection in the database. */ @@ -797,7 +803,15 @@ export class DocumentReference withConverter( converter: PublicFirestoreDataConverter ): PublicDocumentReference { - return new DocumentReference(this.firestore, this._delegate.withConverter(UntypedFirestoreDataConverterAdapter.getInstance(this.firestore, converter))); + return new DocumentReference( + this.firestore, + this._delegate.withConverter( + UntypedFirestoreDataConverterAdapter.getInstance( + this.firestore, + converter + ) + ) + ); } } @@ -1101,7 +1115,15 @@ export class Query } withConverter(converter: PublicFirestoreDataConverter): Query { - return new Query(this.firestore, this._delegate.withConverter(UntypedFirestoreDataConverterAdapter.getInstance(this.firestore, converter))); + return new Query( + this.firestore, + this._delegate.withConverter( + UntypedFirestoreDataConverterAdapter.getInstance( + this.firestore, + converter + ) + ) + ); } } @@ -1240,7 +1262,15 @@ export class CollectionReference withConverter( converter: PublicFirestoreDataConverter ): CollectionReference { - return new CollectionReference(this.firestore, this._delegate.withConverter(UntypedFirestoreDataConverterAdapter.getInstance(this.firestore, converter))); + return new CollectionReference( + this.firestore, + this._delegate.withConverter( + UntypedFirestoreDataConverterAdapter.getInstance( + this.firestore, + converter + ) + ) + ); } } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index cf672fc4231..1a43d3a4497 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1388,7 +1388,7 @@ apiDescribe('Database', (persistence: boolean) => { options: firestore.SnapshotOptions ): ModelWithRef { return new ModelWithRef(snapshot.ref); - }, + } }; it('for DocumentReference.withConverter()', () => { @@ -1621,14 +1621,20 @@ apiDescribe('Database', (persistence: boolean) => { it('Correct snapshot specified to fromFirestore() when registered with CollectionReference', () => { return withTestDb(persistence, async db => { - const untypedCollection = db.collection('/models').doc().collection("sub"); - const collection = untypedCollection.withConverter(modelWithRefConverter); + const untypedCollection = db + .collection('/models') + .doc() + .collection('sub'); + const collection = untypedCollection.withConverter( + modelWithRefConverter + ); const docRef = collection.doc(); await docRef.set(new ModelWithRef(docRef)); const querySnapshot = await collection.get(); expect(querySnapshot.size).to.equal(1); const untypedDocRef = untypedCollection.doc(docRef.id); - expect(untypedDocRef.isEqual(querySnapshot.docs[0].data().ref)).to.be.true; + expect(untypedDocRef.isEqual(querySnapshot.docs[0].data().ref)).to.be + .true; }); }); @@ -1638,10 +1644,13 @@ apiDescribe('Database', (persistence: boolean) => { const untypedDocRef = untypedCollection.doc(); const docRef = untypedDocRef.withConverter(modelWithRefConverter); await docRef.set(new ModelWithRef(docRef)); - const query = untypedCollection.where(FieldPath.documentId(), '==', docRef.id).withConverter(modelWithRefConverter); + const query = untypedCollection + .where(FieldPath.documentId(), '==', docRef.id) + .withConverter(modelWithRefConverter); const querySnapshot = await query.get(); expect(querySnapshot.size).to.equal(1); - expect(untypedDocRef.isEqual(querySnapshot.docs[0].data().ref)).to.be.true; + expect(untypedDocRef.isEqual(querySnapshot.docs[0].data().ref)).to.be + .true; }); }); }); From 0169b06696c82f111a20575227e4faa110dda18a Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 15 Jan 2021 12:58:42 -0500 Subject: [PATCH 06/12] Final cleanups --- packages/firestore/src/api/database.ts | 3 +++ packages/firestore/test/integration/api/database.test.ts | 7 ++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 068f8814473..8e267ada225 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -591,6 +591,9 @@ class UntypedFirestoreDataConverterAdapter } } + // Use the same instance of UntypedFirestoreDataConverterAdapter for a given + // instance of PublicFirestoreDataConverter so that isEqual() will compare + // equal for two objects created with the same converter instance. static getInstance( firestore: Firestore, converter: PublicFirestoreDataConverter diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 1a43d3a4497..353f744bc08 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1377,11 +1377,12 @@ apiDescribe('Database', (persistence: boolean) => { constructor(readonly ref: firestore.DocumentReference) {} } - const omitSingle = (key: any, { [key]: _, ...obj }) => obj; - const modelWithRefConverter = { toFirestore(model: ModelWithRef): firestore.DocumentData { - return omitSingle('ref', model); + // Remove the "ref" attribute, since it's not meant to be stored in + // the Firestore database. + const { ['ref']: _, ...rest } = model; + return rest; }, fromFirestore( snapshot: firestore.QueryDocumentSnapshot, From ef668fd29b968db409612cd77340c1e2bb10d706 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 15 Jan 2021 13:15:16 -0500 Subject: [PATCH 07/12] Rename firestore to _firestore --- packages/firestore/src/api/database.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 8e267ada225..5c8844a1d3a 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -559,7 +559,7 @@ class UntypedFirestoreDataConverterAdapter private static readonly INSTANCES = new WeakMap(); private constructor( - private readonly firestore: Firestore, + private readonly _firestore: Firestore, delegate: PublicFirestoreDataConverter ) { super(delegate); @@ -568,7 +568,7 @@ class UntypedFirestoreDataConverterAdapter fromFirestore(snapshot: unknown, options?: unknown): U { return this._delegate.fromFirestore( new QueryDocumentSnapshot( - this.firestore, + this._firestore, snapshot as ExpQueryDocumentSnapshot ), options as PublicSnapshotOptions From 217f946a51ff6d051385ae996ece47a4f9e2626f Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 15 Jan 2021 13:16:41 -0500 Subject: [PATCH 08/12] Add changeset --- .changeset/neat-yaks-kneel.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/neat-yaks-kneel.md diff --git a/.changeset/neat-yaks-kneel.md b/.changeset/neat-yaks-kneel.md new file mode 100644 index 00000000000..bac9cd21b5a --- /dev/null +++ b/.changeset/neat-yaks-kneel.md @@ -0,0 +1,5 @@ +--- +'@firebase/firestore': patch +--- + +Fixes FirestoreDataConverter.fromFirestore() being called with an incorrect "snapshot" object. From 5f13c375efab3237babb78665025dfb64fe3045c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 18 Jan 2021 15:53:43 -0500 Subject: [PATCH 09/12] Address code review feedback --- packages/firestore/src/api/database.ts | 77 +++++++------------ .../test/integration/api/database.test.ts | 57 +++++--------- 2 files changed, 48 insertions(+), 86 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 5c8844a1d3a..d6b9c4fd2f6 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -553,26 +553,25 @@ export class WriteBatch } } -class UntypedFirestoreDataConverterAdapter - extends Compat> - implements UntypedFirestoreDataConverter { +/** + * Wraps a `PublicFirestoreDataConverter` translating the types from the + * experimental SDK into corresponding types from the Classic SDK before passing + * them to the wrapped converter. + */ +class FirestoreDataConverter extends Compat> implements UntypedFirestoreDataConverter { + private static readonly INSTANCES = new WeakMap(); private constructor( private readonly _firestore: Firestore, + private readonly _userDataWriter: UserDataWriter, delegate: PublicFirestoreDataConverter ) { super(delegate); } - fromFirestore(snapshot: unknown, options?: unknown): U { - return this._delegate.fromFirestore( - new QueryDocumentSnapshot( - this._firestore, - snapshot as ExpQueryDocumentSnapshot - ), - options as PublicSnapshotOptions - ); + fromFirestore(snapshot: ExpQueryDocumentSnapshot, options?: PublicSnapshotOptions): U { + return this._delegate.fromFirestore(new QueryDocumentSnapshot(this._firestore, new ExpQueryDocumentSnapshot(this._firestore._delegate, this._userDataWriter, snapshot._key, snapshot._document, snapshot.metadata,/* converter= */ null)), options ?? {}); } toFirestore(modelObject: U): PublicDocumentData; @@ -591,19 +590,23 @@ class UntypedFirestoreDataConverterAdapter } } - // Use the same instance of UntypedFirestoreDataConverterAdapter for a given - // instance of PublicFirestoreDataConverter so that isEqual() will compare - // equal for two objects created with the same converter instance. - static getInstance( - firestore: Firestore, - converter: PublicFirestoreDataConverter - ): UntypedFirestoreDataConverterAdapter { - const map = UntypedFirestoreDataConverterAdapter.INSTANCES; - let instance = map.get(converter); + // Use the same instance of `FirestoreDataConverter` for the given instances + // of `Firestore` and `PublicFirestoreDataConverter` so that isEqual() will + // compare equal for two objects created with the same converter instance. + static getInstance(firestore: Firestore, converter: PublicFirestoreDataConverter): FirestoreDataConverter { + const converterMapByFirestore = FirestoreDataConverter.INSTANCES; + let untypedConverterByConverter = converterMapByFirestore.get(firestore); + if (!untypedConverterByConverter) { + untypedConverterByConverter = new WeakMap(); + converterMapByFirestore.set(firestore, untypedConverterByConverter); + } + + let instance = untypedConverterByConverter.get(converter); if (!instance) { - instance = new UntypedFirestoreDataConverterAdapter(firestore, converter); - map.set(converter, instance); + instance = new FirestoreDataConverter(firestore, new UserDataWriter(firestore), converter); + untypedConverterByConverter.set(converter, instance); } + return instance; } } @@ -806,15 +809,7 @@ export class DocumentReference withConverter( converter: PublicFirestoreDataConverter ): PublicDocumentReference { - return new DocumentReference( - this.firestore, - this._delegate.withConverter( - UntypedFirestoreDataConverterAdapter.getInstance( - this.firestore, - converter - ) - ) - ); + return new DocumentReference(this.firestore, this._delegate.withConverter(FirestoreDataConverter.getInstance(this.firestore, converter))); } } @@ -1118,15 +1113,7 @@ export class Query } withConverter(converter: PublicFirestoreDataConverter): Query { - return new Query( - this.firestore, - this._delegate.withConverter( - UntypedFirestoreDataConverterAdapter.getInstance( - this.firestore, - converter - ) - ) - ); + return new Query(this.firestore, this._delegate.withConverter(FirestoreDataConverter.getInstance(this.firestore, converter))); } } @@ -1265,15 +1252,7 @@ export class CollectionReference withConverter( converter: PublicFirestoreDataConverter ): CollectionReference { - return new CollectionReference( - this.firestore, - this._delegate.withConverter( - UntypedFirestoreDataConverterAdapter.getInstance( - this.firestore, - converter - ) - ) - ); + return new CollectionReference(this.firestore, this._delegate.withConverter(FirestoreDataConverter.getInstance(this.firestore, converter))); } } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 353f744bc08..f315ef7e5a8 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1326,7 +1326,7 @@ apiDescribe('Database', (persistence: boolean) => { // only to web. apiDescribe('withConverter() support', (persistence: boolean) => { class Post { - constructor(readonly title: string, readonly author: string) {} + constructor(readonly title: string, readonly author: string, readonly ref: firestore.DocumentReference | null = null) {} byline(): string { return this.title + ', by ' + this.author; } @@ -1341,7 +1341,7 @@ apiDescribe('Database', (persistence: boolean) => { options: firestore.SnapshotOptions ): Post { const data = snapshot.data(options); - return new Post(data.title, data.author); + return new Post(data.title, data.author, snapshot.ref); } }; @@ -1369,26 +1369,7 @@ apiDescribe('Database', (persistence: boolean) => { options: firestore.SnapshotOptions ): Post { const data = snapshot.data(); - return new Post(data.title, data.author); - } - }; - - class ModelWithRef { - constructor(readonly ref: firestore.DocumentReference) {} - } - - const modelWithRefConverter = { - toFirestore(model: ModelWithRef): firestore.DocumentData { - // Remove the "ref" attribute, since it's not meant to be stored in - // the Firestore database. - const { ['ref']: _, ...rest } = model; - return rest; - }, - fromFirestore( - snapshot: firestore.QueryDocumentSnapshot, - options: firestore.SnapshotOptions - ): ModelWithRef { - return new ModelWithRef(snapshot.ref); + return new Post(data.title, data.author, snapshot.ref); } }; @@ -1571,7 +1552,7 @@ apiDescribe('Database', (persistence: boolean) => { expect(options).to.deep.equal({ serverTimestamps: 'estimate' }); const data = snapshot.data(options); - return new Post(data.title, data.author); + return new Post(data.title, data.author, snapshot.ref); } }); @@ -1613,10 +1594,12 @@ apiDescribe('Database', (persistence: boolean) => { it('Correct snapshot specified to fromFirestore() when registered with DocumentReference', () => { return withTestDb(persistence, async db => { const untypedDocRef = db.collection('/models').doc(); - const docRef = untypedDocRef.withConverter(modelWithRefConverter); - await docRef.set(new ModelWithRef(docRef)); + const docRef = untypedDocRef.withConverter(postConverter); + await docRef.set(new Post('post', 'author')); const docSnapshot = await docRef.get(); - expect(untypedDocRef.isEqual(docSnapshot.data()!.ref)).to.be.true; + const ref = docSnapshot.data()!.ref!; + //expect(ref).to.be.an.instanceof(firestore.DocumentReference); + expect(untypedDocRef.isEqual(ref)).to.be.true; }); }); @@ -1626,16 +1609,15 @@ apiDescribe('Database', (persistence: boolean) => { .collection('/models') .doc() .collection('sub'); - const collection = untypedCollection.withConverter( - modelWithRefConverter - ); + const collection = untypedCollection.withConverter(postConverter); const docRef = collection.doc(); - await docRef.set(new ModelWithRef(docRef)); + await docRef.set(new Post('post', 'author', docRef)); const querySnapshot = await collection.get(); expect(querySnapshot.size).to.equal(1); + const ref = querySnapshot.docs[0].data().ref!; + //expect(ref).to.be.an.instanceof(firestore.DocumentReference); const untypedDocRef = untypedCollection.doc(docRef.id); - expect(untypedDocRef.isEqual(querySnapshot.docs[0].data().ref)).to.be - .true; + expect(untypedDocRef.isEqual(ref)).to.be.true; }); }); @@ -1643,15 +1625,16 @@ apiDescribe('Database', (persistence: boolean) => { return withTestDb(persistence, async db => { const untypedCollection = db.collection('/models'); const untypedDocRef = untypedCollection.doc(); - const docRef = untypedDocRef.withConverter(modelWithRefConverter); - await docRef.set(new ModelWithRef(docRef)); + const docRef = untypedDocRef.withConverter(postConverter); + await docRef.set(new Post('post', 'author', docRef)); const query = untypedCollection .where(FieldPath.documentId(), '==', docRef.id) - .withConverter(modelWithRefConverter); + .withConverter(postConverter); const querySnapshot = await query.get(); expect(querySnapshot.size).to.equal(1); - expect(untypedDocRef.isEqual(querySnapshot.docs[0].data().ref)).to.be - .true; + const ref = querySnapshot.docs[0].data().ref!; + //expect(ref).to.be.an.instanceof(firestore.DocumentReference); + expect(untypedDocRef.isEqual(ref)).to.be.true; }); }); }); From 3df2e8e49aeb9f2f7917eab632c93f70316dbd15 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 18 Jan 2021 15:53:49 -0500 Subject: [PATCH 10/12] Format code --- packages/firestore/src/api/database.ts | 57 ++++++++++++++++--- .../test/integration/api/database.test.ts | 6 +- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index d6b9c4fd2f6..36fb40cb0d1 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -558,8 +558,9 @@ export class WriteBatch * experimental SDK into corresponding types from the Classic SDK before passing * them to the wrapped converter. */ -class FirestoreDataConverter extends Compat> implements UntypedFirestoreDataConverter { - +class FirestoreDataConverter + extends Compat> + implements UntypedFirestoreDataConverter { private static readonly INSTANCES = new WeakMap(); private constructor( @@ -570,8 +571,24 @@ class FirestoreDataConverter extends Compat> super(delegate); } - fromFirestore(snapshot: ExpQueryDocumentSnapshot, options?: PublicSnapshotOptions): U { - return this._delegate.fromFirestore(new QueryDocumentSnapshot(this._firestore, new ExpQueryDocumentSnapshot(this._firestore._delegate, this._userDataWriter, snapshot._key, snapshot._document, snapshot.metadata,/* converter= */ null)), options ?? {}); + fromFirestore( + snapshot: ExpQueryDocumentSnapshot, + options?: PublicSnapshotOptions + ): U { + return this._delegate.fromFirestore( + new QueryDocumentSnapshot( + this._firestore, + new ExpQueryDocumentSnapshot( + this._firestore._delegate, + this._userDataWriter, + snapshot._key, + snapshot._document, + snapshot.metadata, + /* converter= */ null + ) + ), + options ?? {} + ); } toFirestore(modelObject: U): PublicDocumentData; @@ -593,7 +610,10 @@ class FirestoreDataConverter extends Compat> // Use the same instance of `FirestoreDataConverter` for the given instances // of `Firestore` and `PublicFirestoreDataConverter` so that isEqual() will // compare equal for two objects created with the same converter instance. - static getInstance(firestore: Firestore, converter: PublicFirestoreDataConverter): FirestoreDataConverter { + static getInstance( + firestore: Firestore, + converter: PublicFirestoreDataConverter + ): FirestoreDataConverter { const converterMapByFirestore = FirestoreDataConverter.INSTANCES; let untypedConverterByConverter = converterMapByFirestore.get(firestore); if (!untypedConverterByConverter) { @@ -603,7 +623,11 @@ class FirestoreDataConverter extends Compat> let instance = untypedConverterByConverter.get(converter); if (!instance) { - instance = new FirestoreDataConverter(firestore, new UserDataWriter(firestore), converter); + instance = new FirestoreDataConverter( + firestore, + new UserDataWriter(firestore), + converter + ); untypedConverterByConverter.set(converter, instance); } @@ -809,7 +833,12 @@ export class DocumentReference withConverter( converter: PublicFirestoreDataConverter ): PublicDocumentReference { - return new DocumentReference(this.firestore, this._delegate.withConverter(FirestoreDataConverter.getInstance(this.firestore, converter))); + return new DocumentReference( + this.firestore, + this._delegate.withConverter( + FirestoreDataConverter.getInstance(this.firestore, converter) + ) + ); } } @@ -1113,7 +1142,12 @@ export class Query } withConverter(converter: PublicFirestoreDataConverter): Query { - return new Query(this.firestore, this._delegate.withConverter(FirestoreDataConverter.getInstance(this.firestore, converter))); + return new Query( + this.firestore, + this._delegate.withConverter( + FirestoreDataConverter.getInstance(this.firestore, converter) + ) + ); } } @@ -1252,7 +1286,12 @@ export class CollectionReference withConverter( converter: PublicFirestoreDataConverter ): CollectionReference { - return new CollectionReference(this.firestore, this._delegate.withConverter(FirestoreDataConverter.getInstance(this.firestore, converter))); + return new CollectionReference( + this.firestore, + this._delegate.withConverter( + FirestoreDataConverter.getInstance(this.firestore, converter) + ) + ); } } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index f315ef7e5a8..329da6546d0 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1326,7 +1326,11 @@ apiDescribe('Database', (persistence: boolean) => { // only to web. apiDescribe('withConverter() support', (persistence: boolean) => { class Post { - constructor(readonly title: string, readonly author: string, readonly ref: firestore.DocumentReference | null = null) {} + constructor( + readonly title: string, + readonly author: string, + readonly ref: firestore.DocumentReference | null = null + ) {} byline(): string { return this.title + ', by ' + this.author; } From 926a6174a57b7583d6c8ca2f6d7b896cbbb4c6b6 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 19 Jan 2021 11:37:28 -0500 Subject: [PATCH 11/12] Properly import DocumentReference in database.test.ts --- integration/firestore/firebase_export.ts | 21 ++++++++++++++----- .../firestore/firebase_export_memory.ts | 21 ++++++++++++++----- .../test/integration/api/database.test.ts | 9 +++++--- .../test/integration/util/firebase_export.ts | 17 +++++++++++++-- 4 files changed, 53 insertions(+), 15 deletions(-) diff --git a/integration/firestore/firebase_export.ts b/integration/firestore/firebase_export.ts index 317a53fac29..92d28a23709 100644 --- a/integration/firestore/firebase_export.ts +++ b/integration/firestore/firebase_export.ts @@ -50,11 +50,22 @@ export function usesFunctionalApi(): false { return false; } -const Firestore = firebase.firestore.Firestore; +const Blob = firebase.firestore.Blob; +const DocumentReference = firebase.firestore.DocumentReference; const FieldPath = firebase.firestore.FieldPath; -const Timestamp = firebase.firestore.Timestamp; -const GeoPoint = firebase.firestore.GeoPoint; const FieldValue = firebase.firestore.FieldValue; -const Blob = firebase.firestore.Blob; +const Firestore = firebase.firestore.Firestore; +const GeoPoint = firebase.firestore.GeoPoint; +const QueryDocumentSnapshot = firebase.firestore.QueryDocumentSnapshot; +const Timestamp = firebase.firestore.Timestamp; -export { Firestore, FieldValue, FieldPath, Timestamp, Blob, GeoPoint }; +export { + Blob, + DocumentReference, + FieldPath, + FieldValue, + Firestore, + GeoPoint, + QueryDocumentSnapshot, + Timestamp +}; diff --git a/integration/firestore/firebase_export_memory.ts b/integration/firestore/firebase_export_memory.ts index 98e6ce3ae78..063113e7409 100644 --- a/integration/firestore/firebase_export_memory.ts +++ b/integration/firestore/firebase_export_memory.ts @@ -50,11 +50,22 @@ export function usesFunctionalApi(): false { return false; } -const Firestore = firebase.firestore.Firestore; +const Blob = firebase.firestore.Blob; +const DocumentReference = firebase.firestore.DocumentReference; const FieldPath = firebase.firestore.FieldPath; -const Timestamp = firebase.firestore.Timestamp; -const GeoPoint = firebase.firestore.GeoPoint; const FieldValue = firebase.firestore.FieldValue; -const Blob = firebase.firestore.Blob; +const Firestore = firebase.firestore.Firestore; +const GeoPoint = firebase.firestore.GeoPoint; +const QueryDocumentSnapshot = firebase.firestore.QueryDocumentSnapshot; +const Timestamp = firebase.firestore.Timestamp; -export { Firestore, FieldValue, FieldPath, Timestamp, Blob, GeoPoint }; +export { + Blob, + DocumentReference, + FieldPath, + FieldValue, + Firestore, + GeoPoint, + QueryDocumentSnapshot, + Timestamp +}; diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 329da6546d0..e8c2b0f6b0f 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -39,6 +39,8 @@ const newTestFirestore = firebaseExport.newTestFirestore; const Timestamp = firebaseExport.Timestamp; const FieldPath = firebaseExport.FieldPath; const FieldValue = firebaseExport.FieldValue; +const DocumentReference = firebaseExport.DocumentReference; +const QueryDocumentSnapshot = firebaseExport.QueryDocumentSnapshot; const MEMORY_ONLY_BUILD = typeof process !== 'undefined' && @@ -1344,6 +1346,7 @@ apiDescribe('Database', (persistence: boolean) => { snapshot: firestore.QueryDocumentSnapshot, options: firestore.SnapshotOptions ): Post { + expect(snapshot).to.be.an.instanceof(QueryDocumentSnapshot); const data = snapshot.data(options); return new Post(data.title, data.author, snapshot.ref); } @@ -1602,7 +1605,7 @@ apiDescribe('Database', (persistence: boolean) => { await docRef.set(new Post('post', 'author')); const docSnapshot = await docRef.get(); const ref = docSnapshot.data()!.ref!; - //expect(ref).to.be.an.instanceof(firestore.DocumentReference); + expect(ref).to.be.an.instanceof(DocumentReference); expect(untypedDocRef.isEqual(ref)).to.be.true; }); }); @@ -1619,7 +1622,7 @@ apiDescribe('Database', (persistence: boolean) => { const querySnapshot = await collection.get(); expect(querySnapshot.size).to.equal(1); const ref = querySnapshot.docs[0].data().ref!; - //expect(ref).to.be.an.instanceof(firestore.DocumentReference); + expect(ref).to.be.an.instanceof(DocumentReference); const untypedDocRef = untypedCollection.doc(docRef.id); expect(untypedDocRef.isEqual(ref)).to.be.true; }); @@ -1637,7 +1640,7 @@ apiDescribe('Database', (persistence: boolean) => { const querySnapshot = await query.get(); expect(querySnapshot.size).to.equal(1); const ref = querySnapshot.docs[0].data().ref!; - //expect(ref).to.be.an.instanceof(firestore.DocumentReference); + expect(ref).to.be.an.instanceof(DocumentReference); expect(untypedDocRef.isEqual(ref)).to.be.true; }); }); diff --git a/packages/firestore/test/integration/util/firebase_export.ts b/packages/firestore/test/integration/util/firebase_export.ts index 50a424d4bf2..b6d825c900b 100644 --- a/packages/firestore/test/integration/util/firebase_export.ts +++ b/packages/firestore/test/integration/util/firebase_export.ts @@ -25,7 +25,11 @@ import { FirebaseApp } from '@firebase/app-types'; import * as firestore from '@firebase/firestore-types'; import { Blob } from '../../../src/api/blob'; -import { Firestore } from '../../../src/api/database'; +import { + Firestore, + DocumentReference, + QueryDocumentSnapshot +} from '../../../src/api/database'; import { FieldPath } from '../../../src/api/field_path'; import { FieldValue } from '../../../src/api/field_value'; import { GeoPoint } from '../../../src/api/geo_point'; @@ -68,4 +72,13 @@ export function newTestFirestore( return firestore; } -export { Firestore, FieldValue, FieldPath, Timestamp, Blob, GeoPoint }; +export { + Firestore, + FieldValue, + FieldPath, + Timestamp, + Blob, + GeoPoint, + DocumentReference, + QueryDocumentSnapshot +}; From 4aa48fe8dfc0b1ee8611c28c31a01f512eea0ea0 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 19 Jan 2021 12:17:46 -0500 Subject: [PATCH 12/12] Fix incorrect use of typed QueryDocumentSnapshot and ExpQueryDocumentSnapshot in fromFirestore --- packages/firestore/src/api/database.ts | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 36fb40cb0d1..7a44c5834dd 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -575,18 +575,16 @@ class FirestoreDataConverter snapshot: ExpQueryDocumentSnapshot, options?: PublicSnapshotOptions ): U { + const expSnapshot = new ExpQueryDocumentSnapshot( + this._firestore._delegate, + this._userDataWriter, + snapshot._key, + snapshot._document, + snapshot.metadata, + /* converter= */ null + ); return this._delegate.fromFirestore( - new QueryDocumentSnapshot( - this._firestore, - new ExpQueryDocumentSnapshot( - this._firestore._delegate, - this._userDataWriter, - snapshot._key, - snapshot._document, - snapshot.metadata, - /* converter= */ null - ) - ), + new QueryDocumentSnapshot(this._firestore, expSnapshot), options ?? {} ); }