From 83a7bfe5debdf405fa191e565cba70c87c96c016 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Tue, 2 Mar 2021 16:49:55 -0600 Subject: [PATCH 1/4] Add default converter option in withConverter() --- packages/firebase/index.d.ts | 61 ++++++++++++++++++- packages/firestore-types/index.d.ts | 9 ++- packages/firestore/src/api/database.ts | 48 ++++++++++----- packages/firestore/src/lite/reference.ts | 20 +++++- .../test/integration/api/database.test.ts | 37 +++++++++++ .../firestore/test/lite/integration.test.ts | 45 +++++++++++--- 6 files changed, 189 insertions(+), 31 deletions(-) diff --git a/packages/firebase/index.d.ts b/packages/firebase/index.d.ts index 6c43a186ca9..d33bd26541b 100644 --- a/packages/firebase/index.d.ts +++ b/packages/firebase/index.d.ts @@ -9089,7 +9089,26 @@ declare namespace firebase.firestore { * provided converter will convert between Firestore data and your custom * type U. * - * @param converter Converts objects to and from Firestore. + * Passing in `null` as the converter parameter removes the current + * converter. + * + * @param converter Converts objects to and from Firestore. Passing in + * `null` removes the current converter. + * @return A DocumentReference that uses the provided converter. + */ + withConverter(converter: null): DocumentReference; + /** + * Applies a custom data converter to this DocumentReference, allowing you + * to use your own custom model objects with Firestore. When you call + * set(), get(), etc. on the returned DocumentReference instance, the + * provided converter will convert between Firestore data and your custom + * type U. + * + * Passing in `null` as the converter parameter removes the current + * converter. + * + * @param converter Converts objects to and from Firestore. Passing in + * `null` removes the current converter. * @return A DocumentReference that uses the provided converter. */ withConverter( @@ -9544,7 +9563,25 @@ declare namespace firebase.firestore { * returned Query, the provided converter will convert between Firestore * data and your custom type U. * - * @param converter Converts objects to and from Firestore. + * Passing in `null` as the converter parameter removes the current + * converter. + * + * @param converter Converts objects to and from Firestore. Passing in + * `null` removes the current converter. + * @return A Query that uses the provided converter. + */ + withConverter(converter: null): Query; + /** + * Applies a custom data converter to this Query, allowing you to use your + * own custom model objects with Firestore. When you call get() on the + * returned Query, the provided converter will convert between Firestore + * data and your custom type U. + * + * Passing in `null` as the converter parameter removes the current + * converter. + * + * @param converter Converts objects to and from Firestore. Passing in + * `null` removes the current converter. * @return A Query that uses the provided converter. */ withConverter(converter: FirestoreDataConverter): Query; @@ -9700,7 +9737,25 @@ declare namespace firebase.firestore { * on the returned CollectionReference instance, the provided converter will * convert between Firestore data and your custom type U. * - * @param converter Converts objects to and from Firestore. + * Passing in `null` as the converter parameter removes the current + * converter. + * + * @param converter Converts objects to and from Firestore. Passing in + * `null` removes the current converter. + * @return A CollectionReference that uses the provided converter. + */ + withConverter(converter: null): CollectionReference; + /** + * Applies a custom data converter to this CollectionReference, allowing you + * to use your own custom model objects with Firestore. When you call add() + * on the returned CollectionReference instance, the provided converter will + * convert between Firestore data and your custom type U. + * + * Passing in `null` as the converter parameter removes the current + * converter. + * + * @param converter Converts objects to and from Firestore. Passing in + * `null` removes the current converter. * @return A CollectionReference that uses the provided converter. */ withConverter( diff --git a/packages/firestore-types/index.d.ts b/packages/firestore-types/index.d.ts index 9722edee019..13232f20a53 100644 --- a/packages/firestore-types/index.d.ts +++ b/packages/firestore-types/index.d.ts @@ -286,6 +286,7 @@ export class DocumentReference { onCompletion?: () => void ): () => void; + withConverter(converter: null): DocumentReference; withConverter(converter: FirestoreDataConverter): DocumentReference; } @@ -315,9 +316,9 @@ export class DocumentSnapshot { isEqual(other: DocumentSnapshot): boolean; } -export class QueryDocumentSnapshot extends DocumentSnapshot< - T -> { +export class QueryDocumentSnapshot< + T = DocumentData +> extends DocumentSnapshot { private constructor(); data(options?: SnapshotOptions): T; @@ -398,6 +399,7 @@ export class Query { onCompletion?: () => void ): () => void; + withConverter(converter: null): Query; withConverter(converter: FirestoreDataConverter): Query; } @@ -442,6 +444,7 @@ export class CollectionReference extends Query { isEqual(other: CollectionReference): boolean; + withConverter(converter: null): CollectionReference; withConverter( converter: FirestoreDataConverter ): CollectionReference; diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 2af044f7a79..2e0bd434830 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -830,14 +830,20 @@ export class DocumentReference ); } + withConverter(converter: null): PublicDocumentReference; withConverter( converter: PublicFirestoreDataConverter - ): PublicDocumentReference { - return new DocumentReference( + ): PublicDocumentReference; + withConverter( + converter: PublicFirestoreDataConverter | null + ): PublicDocumentReference | PublicDocumentReference { + return new DocumentReference( this.firestore, - this._delegate.withConverter( - FirestoreDataConverter.getInstance(this.firestore, converter) - ) + converter + ? this._delegate.withConverter( + FirestoreDataConverter.getInstance(this.firestore, converter) + ) + : this._delegate.withConverter(null) ); } } @@ -1141,12 +1147,18 @@ export class Query return onSnapshot(this._delegate, options, observer); } - withConverter(converter: PublicFirestoreDataConverter): Query { - return new Query( + withConverter(converter: null): Query; + withConverter(converter: PublicFirestoreDataConverter): Query; + withConverter( + converter: PublicFirestoreDataConverter | null + ): Query | Query { + return new Query( this.firestore, - this._delegate.withConverter( - FirestoreDataConverter.getInstance(this.firestore, converter) - ) + converter + ? this._delegate.withConverter( + FirestoreDataConverter.getInstance(this.firestore, converter) + ) + : this._delegate.withConverter(null) ); } } @@ -1283,14 +1295,20 @@ export class CollectionReference return refEqual(this._delegate, other._delegate); } + withConverter(converter: null): CollectionReference; withConverter( converter: PublicFirestoreDataConverter - ): CollectionReference { - return new CollectionReference( + ): CollectionReference; + withConverter( + converter: PublicFirestoreDataConverter | null + ): CollectionReference | CollectionReference { + return new CollectionReference( this.firestore, - this._delegate.withConverter( - FirestoreDataConverter.getInstance(this.firestore, converter) - ) + converter + ? this._delegate.withConverter( + FirestoreDataConverter.getInstance(this.firestore, converter) + ) + : this._delegate.withConverter(null) ); } } diff --git a/packages/firestore/src/lite/reference.ts b/packages/firestore/src/lite/reference.ts index 9d07479c904..eba68a0f177 100644 --- a/packages/firestore/src/lite/reference.ts +++ b/packages/firestore/src/lite/reference.ts @@ -138,10 +138,18 @@ export class DocumentReference { * instance, the provided converter will convert between Firestore data and * your custom type `U`. * - * @param converter - Converts objects to and from Firestore. + * Passing in `null` as the converter parameter removes the current + * converter. + * + * @param converter - Converts objects to and from Firestore. Passing in + * `null` removes the current converter. * @returns A `DocumentReference` that uses the provided converter. */ - withConverter(converter: FirestoreDataConverter): DocumentReference { + withConverter(converter: null): DocumentReference; + withConverter(converter: FirestoreDataConverter): DocumentReference; + withConverter( + converter: FirestoreDataConverter | null + ): DocumentReference { return new DocumentReference(this.firestore, converter, this._key); } } @@ -180,7 +188,9 @@ export class Query { * @param converter - Converts objects to and from Firestore. * @returns A `Query` that uses the provided converter. */ - withConverter(converter: FirestoreDataConverter): Query { + withConverter(converter: null): Query; + withConverter(converter: FirestoreDataConverter): Query; + withConverter(converter: FirestoreDataConverter | null): Query { return new Query(this.firestore, converter, this._query); } } @@ -240,8 +250,12 @@ export class CollectionReference extends Query { * @param converter - Converts objects to and from Firestore. * @returns A `CollectionReference` that uses the provided converter. */ + withConverter(converter: null): CollectionReference; withConverter( converter: FirestoreDataConverter + ): CollectionReference; + withConverter( + converter: FirestoreDataConverter | null ): CollectionReference { return new CollectionReference(this.firestore, converter, this._path); } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index e8c2b0f6b0f..a4900c564fe 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1395,6 +1395,18 @@ apiDescribe('Database', (persistence: boolean) => { }); }); + it('for DocumentReference.withConverter(null) ', () => { + return withTestDb(persistence, async db => { + const docRef = db + .collection('posts') + .doc() + .withConverter(postConverter) + .withConverter(null); + + expect(() => docRef.set(new Post('post', 'author'))).to.throw(); + }); + }); + it('for CollectionReference.withConverter()', () => { return withTestDb(persistence, async db => { const coll = db.collection('posts').withConverter(postConverter); @@ -1407,6 +1419,17 @@ apiDescribe('Database', (persistence: boolean) => { }); }); + it('for CollectionReference.withConverter(null)', () => { + return withTestDb(persistence, async db => { + const coll = db + .collection('posts') + .withConverter(postConverter) + .withConverter(null); + + expect(() => coll.add(new Post('post', 'author'))).to.throw(); + }); + }); + it('for Query.withConverter()', () => { return withTestDb(persistence, async db => { await db @@ -1424,6 +1447,20 @@ apiDescribe('Database', (persistence: boolean) => { }); }); + it('for Query.withConverter(null)', () => { + return withTestDb(persistence, async db => { + await db + .doc('postings/post1') + .set({ title: 'post1', author: 'author1' }); + const posts = await db + .collectionGroup('postings') + .withConverter(postConverter) + .withConverter(null) + .get(); + expect(posts.docs[0].data()).to.not.be.an.instanceof(Post); + }); + }); + it('requires the correct converter for Partial usage', async () => { return withTestDb(persistence, async db => { const ref = db diff --git a/packages/firestore/test/lite/integration.test.ts b/packages/firestore/test/lite/integration.test.ts index 25d4bcf2d5f..fe5296b4714 100644 --- a/packages/firestore/test/lite/integration.test.ts +++ b/packages/firestore/test/lite/integration.test.ts @@ -1114,6 +1114,15 @@ describe('withConverter() support', () => { }); }); + it('for DocumentReference.withConverter(null) applies default converter', () => { + return withTestCollection(async coll => { + coll = coll.withConverter(postConverter).withConverter(null); + expect(() => + setDoc(doc(coll, 'post1'), new Post('post', 'author')) + ).to.throw(); + }); + }); + it('for CollectionReference.withConverter()', () => { return withTestCollection(async coll => { coll = coll.withConverter(postConverter); @@ -1125,16 +1134,38 @@ describe('withConverter() support', () => { }); }); - it('for Query.withConverter()', () => { - return withTestCollection(async coll => { - coll = coll.withConverter(postConverter); - await setDoc(doc(coll, 'post1'), new Post('post1', 'author1')); - const posts = await getDocs(coll); - expect(posts.size).to.equal(1); - expect(posts.docs[0].data()!.byline()).to.equal('post1, by author1'); + it('for CollectionReference.withConverter(null) applies default converter', () => { + return withTestDoc(async doc => { + doc = doc.withConverter(postConverter).withConverter(null); + expect(() => setDoc(doc, new Post('post', 'author'))).to.throw(); }); }); + it('for Query.withConverter()', () => { + return withTestCollectionAndInitialData( + [{ title: 'post', author: 'author' }], + async collRef => { + let query1 = query(collRef, where('title', '==', 'post')); + query1 = query1.withConverter(postConverter); + const result = await getDocs(query1); + expect(result.docs[0].data()).to.be.an.instanceOf(Post); + expect(result.docs[0].data()!.byline()).to.equal('post, by author'); + } + ); + }); + + it('for Query.withConverter(null) applies default converter', () => { + return withTestCollectionAndInitialData( + [{ title: 'post', author: 'author' }], + async collRef => { + let query1 = query(collRef, where('title', '==', 'post')); + query1 = query1.withConverter(postConverter).withConverter(null); + const result = await getDocs(query1); + expect(result.docs[0]).to.not.be.an.instanceOf(Post); + } + ); + }); + it('keeps the converter when calling parent() with a DocumentReference', () => { return withTestDb(async db => { const coll = doc(db, 'root/doc').withConverter(postConverter); From e7391cd8c3962ee8e4adc3e5c1cadcfea2d8fa12 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Tue, 2 Mar 2021 16:59:29 -0600 Subject: [PATCH 2/4] add changeset --- .changeset/lemon-laws-brush.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/lemon-laws-brush.md diff --git a/.changeset/lemon-laws-brush.md b/.changeset/lemon-laws-brush.md new file mode 100644 index 00000000000..ce59a01d495 --- /dev/null +++ b/.changeset/lemon-laws-brush.md @@ -0,0 +1,7 @@ +--- +'firebase': patch +'@firebase/firestore': patch +'@firebase/firestore-types': patch +--- + +Added support to remove a FirestoreDataConverter on a Firestore reference by calling `withConverter(null)` From 47f882d7654fd457b5e07fdd7067ca176a414ebf Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 3 Mar 2021 11:33:55 -0600 Subject: [PATCH 3/4] failing: attempt using default in DocumentReference converter --- packages/firestore/src/api/database.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 2e0bd434830..7dcef8674ed 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -836,11 +836,11 @@ export class DocumentReference ): PublicDocumentReference; withConverter( converter: PublicFirestoreDataConverter | null - ): PublicDocumentReference | PublicDocumentReference { - return new DocumentReference( + ): PublicDocumentReference { + return new DocumentReference( this.firestore, converter - ? this._delegate.withConverter( + ? this._delegate.withConverter( FirestoreDataConverter.getInstance(this.firestore, converter) ) : this._delegate.withConverter(null) From 45aba1645e5250e2e75400898a1c4338e2aeff91 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 3 Mar 2021 12:05:36 -0600 Subject: [PATCH 4/4] apply sebastian big brain --- packages/firestore/src/api/database.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 7dcef8674ed..dbbf5ad7fe8 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -840,10 +840,10 @@ export class DocumentReference return new DocumentReference( this.firestore, converter - ? this._delegate.withConverter( + ? this._delegate.withConverter( FirestoreDataConverter.getInstance(this.firestore, converter) ) - : this._delegate.withConverter(null) + : (this._delegate.withConverter(null) as ExpDocumentReference) ); } } @@ -1151,14 +1151,14 @@ export class Query withConverter(converter: PublicFirestoreDataConverter): Query; withConverter( converter: PublicFirestoreDataConverter | null - ): Query | Query { - return new Query( + ): Query { + return new Query( this.firestore, converter ? this._delegate.withConverter( FirestoreDataConverter.getInstance(this.firestore, converter) ) - : this._delegate.withConverter(null) + : (this._delegate.withConverter(null) as ExpQuery) ); } } @@ -1301,14 +1301,14 @@ export class CollectionReference ): CollectionReference; withConverter( converter: PublicFirestoreDataConverter | null - ): CollectionReference | CollectionReference { - return new CollectionReference( + ): CollectionReference { + return new CollectionReference( this.firestore, converter ? this._delegate.withConverter( FirestoreDataConverter.getInstance(this.firestore, converter) ) - : this._delegate.withConverter(null) + : (this._delegate.withConverter(null) as ExpCollectionReference) ); } }