From f6c62da3ef7cbdfbffac9df75213f22c31f2f0e7 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Mon, 22 Jun 2020 16:22:01 -0700 Subject: [PATCH 01/18] add set override --- packages/firebase/index.d.ts | 68 ++++++++-- packages/firestore-types/index.d.ts | 31 ++++- packages/firestore/src/api/database.ts | 45 +++++-- .../test/integration/api/database.test.ts | 126 ++++++++++++++++++ 4 files changed, 244 insertions(+), 26 deletions(-) diff --git a/packages/firebase/index.d.ts b/packages/firebase/index.d.ts index 93df8ad400f..65f1d06fa2a 100644 --- a/packages/firebase/index.d.ts +++ b/packages/firebase/index.d.ts @@ -15,6 +15,8 @@ * limitations under the License. */ +import { DocumentData } from '@firebase/firestore-types'; + /** * firebase is a global namespace from which all Firebase * services are accessed. @@ -7859,9 +7861,12 @@ declare namespace firebase.firestore { /** * Called by the Firestore SDK to convert a custom model object of type T * into a plain Javascript object (suitable for writing directly to the - * Firestore database). + * Firestore database). To use set() with `merge` and `mergeFields, + * toFirestore() must be defined with `Partial`. */ - toFirestore(modelObject: T): DocumentData; + toFirestore: + | ((modelObject: T) => DocumentData) + | ((modelObject: Partial, options?: SetOptions) => DocumentData); /** * Called by the Firestore SDK to convert Firestore data into an object of @@ -7870,7 +7875,10 @@ declare namespace firebase.firestore { * @param snapshot A QueryDocumentSnapshot containing your data and metadata. * @param options The SnapshotOptions from the initial call to `data()`. */ - fromFirestore(snapshot: QueryDocumentSnapshot, options: SnapshotOptions): T; + fromFirestore: ( + snapshot: QueryDocumentSnapshot, + options: SnapshotOptions + ) => T; } /** @@ -8310,10 +8318,32 @@ declare namespace firebase.firestore { */ set( documentRef: DocumentReference, - data: T, - options?: SetOptions + data: Partial, + options: SetOptions ): Transaction; + /** + * Writes to the document referred to by the provided `DocumentReference`. + * If the document does not exist yet, it will be created. If you pass + * `SetOptions`, the provided data can be merged into the existing document. + * + * @param documentRef A reference to the document to be set. + * @param data An object of the fields and values for the document. + * @return This `Transaction` instance. Used for chaining method calls. + */ + set(documentRef: DocumentReference, data: T): Transaction; + + /** + * Writes to the document referred to by the provided `DocumentReference`. + * If the document does not exist yet, it will be created. If you pass + * `SetOptions`, the provided data can be merged into the existing document. + * + * @param documentRef A reference to the document to be set. + * @param data An object of the fields and values for the document. + * @param options An object to configure the set behavior. + * @return This `Transaction` instance. Used for chaining method calls. + */ + /** * Updates fields in the document referred to by the provided * `DocumentReference`. The update will fail if applied to a document that @@ -8384,10 +8414,21 @@ declare namespace firebase.firestore { */ set( documentRef: DocumentReference, - data: T, - options?: SetOptions + data: Partial, + options: SetOptions ): WriteBatch; + /** + * Writes to the document referred to by the provided `DocumentReference`. + * If the document does not exist yet, it will be created. If you pass + * `SetOptions`, the provided data can be merged into the existing document. + * + * @param documentRef A reference to the document to be set. + * @param data An object of the fields and values for the document. + * @return This `WriteBatch` instance. Used for chaining method calls. + */ + set(documentRef: DocumentReference, data: T): WriteBatch; + /** * Updates fields in the document referred to by the provided * `DocumentReference`. The update will fail if applied to a document that @@ -8566,7 +8607,18 @@ declare namespace firebase.firestore { * @return A Promise resolved once the data has been successfully written * to the backend (Note that it won't resolve while you're offline). */ - set(data: T, options?: SetOptions): Promise; + set(data: Partial, options: SetOptions): Promise; + + /** + * Writes to the document referred to by this `DocumentReference`. If the + * document does not yet exist, it will be created. If you pass + * `SetOptions`, the provided data can be merged into an existing document. + * + * @param data A map of the fields and values for the document. + * @return A Promise resolved once the data has been successfully written + * to the backend (Note that it won't resolve while you're offline). + */ + set(data: T): Promise; /** * Updates fields in the document referred to by this `DocumentReference`. diff --git a/packages/firestore-types/index.d.ts b/packages/firestore-types/index.d.ts index d264cacf795..d4f3b84c106 100644 --- a/packages/firestore-types/index.d.ts +++ b/packages/firestore-types/index.d.ts @@ -49,9 +49,14 @@ export type LogLevel = export function setLogLevel(logLevel: LogLevel): void; export interface FirestoreDataConverter { - toFirestore(modelObject: T): DocumentData; - - fromFirestore(snapshot: QueryDocumentSnapshot, options: SnapshotOptions): T; + toFirestore: + | ((modelObject: T) => DocumentData) + | ((modelObject: Partial, options?: SetOptions) => DocumentData); + + fromFirestore: ( + snapshot: QueryDocumentSnapshot, + options: SnapshotOptions + ) => T; } export class FirebaseFirestore { @@ -146,7 +151,13 @@ export class Transaction { set( documentRef: DocumentReference, - data: T, + data: Partial, + options: SetOptions + ): Transaction; + set(documentRef: DocumentReference, data: T): Transaction; + set( + documentRef: DocumentReference, + data: T | Partial, options?: SetOptions ): Transaction; @@ -166,7 +177,13 @@ export class WriteBatch { set( documentRef: DocumentReference, - data: T, + data: Partial, + options: SetOptions + ): WriteBatch; + set(documentRef: DocumentReference, data: T): WriteBatch; + set( + documentRef: DocumentReference, + data: T | Partial, options?: SetOptions ): WriteBatch; @@ -208,7 +225,9 @@ export class DocumentReference { isEqual(other: DocumentReference): boolean; - set(data: T, options?: SetOptions): Promise; + set(data: Partial, options: SetOptions): Promise; + set(data: T): Promise; + set(data: T | Partial, options?: SetOptions): Promise; update(data: UpdateData): Promise; update( diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index cc56523fd50..00bbf1af8b4 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -726,9 +726,15 @@ export class Transaction implements firestore.Transaction { }); } + set( + documentRef: DocumentReference, + data: Partial, + options: firestore.SetOptions + ): Transaction; + set(documentRef: DocumentReference, data: T): Transaction; set( documentRef: firestore.DocumentReference, - value: T, + value: T | Partial, options?: firestore.SetOptions ): Transaction { validateBetweenNumberOfArgs('Transaction.set', arguments, 2, 3); @@ -741,7 +747,8 @@ export class Transaction implements firestore.Transaction { const [convertedValue, functionName] = applyFirestoreDataConverter( ref._converter, value, - 'Transaction.set' + 'Transaction.set', + options ); const parsed = this._firestore._dataReader.parseSetData( functionName, @@ -822,9 +829,15 @@ export class WriteBatch implements firestore.WriteBatch { constructor(private _firestore: Firestore) {} + set( + documentRef: DocumentReference, + data: Partial, + options: firestore.SetOptions + ): WriteBatch; + set(documentRef: DocumentReference, data: T): WriteBatch; set( documentRef: firestore.DocumentReference, - value: T, + value: T | Partial, options?: firestore.SetOptions ): WriteBatch { validateBetweenNumberOfArgs('WriteBatch.set', arguments, 2, 3); @@ -838,7 +851,8 @@ export class WriteBatch implements firestore.WriteBatch { const [convertedValue, functionName] = applyFirestoreDataConverter( ref._converter, value, - 'WriteBatch.set' + 'WriteBatch.set', + options ); const parsed = this._firestore._dataReader.parseSetData( functionName, @@ -1026,17 +1040,16 @@ export class DocumentReference ); } - set( - value: firestore.DocumentData, - options?: firestore.SetOptions - ): Promise; - set(value: T, options?: firestore.SetOptions): Promise { + set(value: Partial, options: firestore.SetOptions): Promise; + set(value: T): Promise; + set(value: T | Partial, options?: firestore.SetOptions): Promise { validateBetweenNumberOfArgs('DocumentReference.set', arguments, 1, 2); options = validateSetOptions('DocumentReference.set', options); const [convertedValue, functionName] = applyFirestoreDataConverter( this._converter, value, - 'DocumentReference.set' + 'DocumentReference.set', + options ); const parsed = this.firestore._dataReader.parseSetData( functionName, @@ -2573,11 +2586,19 @@ function resultChangeType(type: ChangeType): firestore.DocumentChangeType { export function applyFirestoreDataConverter( converter: UntypedFirestoreDataConverter | null, value: T, - functionName: string + functionName: string, + options?: firestore.SetOptions ): [firestore.DocumentData, string] { let convertedValue; if (converter) { - convertedValue = converter.toFirestore(value); + if (options && (options.merge || options.mergeFields)) { + // Cast to `any` in order to satisfy the union type constraint on + // toFirestore(). + // eslint-disable-next-line @typescript-eslint/no-explicit-any + convertedValue = (converter as any).toFirestore(value, options); + } else { + convertedValue = converter.toFirestore(value); + } functionName = 'toFirestore() in ' + functionName; } else { convertedValue = value as firestore.DocumentData; diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 33685ab2900..76aa40cea50 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1296,6 +1296,34 @@ apiDescribe('Database', (persistence: boolean) => { } }; + const postConverterMerge = { + toFirestore( + post: Partial, + options?: firestore.SetOptions + ): firestore.DocumentData { + if (options && (options.merge || options.mergeFields)) { + expect(post).to.not.be.an.instanceof(Post); + } else { + expect(post).to.be.an.instanceof(Post); + } + const result: firestore.DocumentData = {}; + if (post.title) { + result.title = post.title; + } + if (post.author) { + result.author = post.author; + } + return result; + }, + fromFirestore( + snapshot: firestore.QueryDocumentSnapshot, + options: firestore.SnapshotOptions + ): Post { + const data = snapshot.data(); + return new Post(data.title, data.author); + } + }; + it('for DocumentReference.withConverter()', () => { return withTestDb(persistence, async db => { const docRef = db @@ -1340,6 +1368,104 @@ apiDescribe('Database', (persistence: boolean) => { }); }); + it('requires the correct converter for Partial usage', async () => { + return withTestDb(persistence, async db => { + const ref = db.doc('postings/post1').withConverter(postConverter); + await ref.set(new Post('walnut', 'author')); + const batch = db.batch(); + expect(() => + batch.set(ref, { title: 'olive' }, { merge: true }) + ).to.throw( + 'Function toFirestore() in WriteBatch.set() called with invalid data. Unsupported field value: undefined (found in field author)' + ); + }); + }); + + it('WriteBatch.set() supports partials with merge', async () => { + return withTestDb(persistence, async db => { + const ref = db.doc('postings/post1').withConverter(postConverterMerge); + await ref.set(new Post('walnut', 'author')); + const batch = db.batch(); + batch.set(ref, { title: 'olive' }, { merge: true }); + await batch.commit(); + const doc = await ref.get(); + expect(doc.get('title')).to.equal('olive'); + expect(doc.get('author')).to.equal('author'); + }); + }); + + it('WriteBatch.set() supports partials with mergeFields', async () => { + return withTestDb(persistence, async db => { + const ref = db.doc('postings/post1').withConverter(postConverterMerge); + await ref.set(new Post('walnut', 'author')); + const batch = db.batch(); + batch.set( + ref, + { title: 'olive', author: 'writer' }, + { mergeFields: ['title'] } + ); + await batch.commit(); + const doc = await ref.get(); + expect(doc.get('title')).to.equal('olive'); + expect(doc.get('author')).to.equal('author'); + }); + }); + + it('Transaction.set() supports partials with merge', async () => { + return withTestDb(persistence, async db => { + const ref = db.doc('postings/post1').withConverter(postConverterMerge); + await ref.set(new Post('walnut', 'author')); + await db.runTransaction(async tx => { + tx.set(ref, { title: 'olive' }, { merge: true }); + }); + const doc = await ref.get(); + expect(doc.get('title')).to.equal('olive'); + expect(doc.get('author')).to.equal('author'); + }); + }); + + it('Transaction.set() supports partials with mergeFields', async () => { + return withTestDb(persistence, async db => { + const ref = db.doc('postings/post1').withConverter(postConverterMerge); + await ref.set(new Post('walnut', 'author')); + await db.runTransaction(async tx => { + tx.set( + ref, + { title: 'olive', author: 'person' }, + { mergeFields: ['title'] } + ); + }); + const doc = await ref.get(); + expect(doc.get('title')).to.equal('olive'); + expect(doc.get('author')).to.equal('author'); + }); + }); + + it('DocumentReference.set() supports partials with merge', async () => { + return withTestDb(persistence, async db => { + const ref = db.doc('postings/post1').withConverter(postConverterMerge); + await ref.set(new Post('walnut', 'author')); + await ref.set({ title: 'olive' }, { merge: true }); + const doc = await ref.get(); + expect(doc.get('title')).to.equal('olive'); + expect(doc.get('author')).to.equal('author'); + }); + }); + + it('DocumentReference.set() supports partials with mergeFields', async () => { + return withTestDb(persistence, async db => { + const ref = db.doc('postings/post1').withConverter(postConverterMerge); + await ref.set(new Post('walnut', 'author')); + await ref.set( + { title: 'olive', author: 'writer' }, + { mergeFields: ['title'] } + ); + const doc = await ref.get(); + expect(doc.get('title')).to.equal('olive'); + expect(doc.get('author')).to.equal('author'); + }); + }); + it('calls DocumentSnapshot.data() with specified SnapshotOptions', () => { return withTestDb(persistence, async db => { const docRef = db.doc('some/doc').withConverter({ From 392751267df714d8f5e8bd11a92dc9cde9d2f175 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Mon, 22 Jun 2020 16:35:09 -0700 Subject: [PATCH 02/18] add changelog --- packages/firestore/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/firestore/CHANGELOG.md b/packages/firestore/CHANGELOG.md index c9aa89edfdb..0553fb109c3 100644 --- a/packages/firestore/CHANGELOG.md +++ b/packages/firestore/CHANGELOG.md @@ -1,6 +1,8 @@ # Unreleased - [fixed] Fixed an issue that may have prevented the client from connecting to the backend immediately after a user signed in. +- [fixed] Added support for using `set()` with merge options when using + `FirestoreDataConverter`. # Released - [changed] All known failure cases for Indexed-related crashes have now been From 1fd7d6648585e6e74b766ef68e39f40dfc15a415 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Mon, 22 Jun 2020 17:20:54 -0700 Subject: [PATCH 03/18] use a new doc each time to avoid flakes --- .../test/integration/api/database.test.ts | 35 +++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 76aa40cea50..99e0c07e89b 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1370,7 +1370,10 @@ apiDescribe('Database', (persistence: boolean) => { it('requires the correct converter for Partial usage', async () => { return withTestDb(persistence, async db => { - const ref = db.doc('postings/post1').withConverter(postConverter); + const ref = db + .collection('posts') + .doc() + .withConverter(postConverter); await ref.set(new Post('walnut', 'author')); const batch = db.batch(); expect(() => @@ -1383,7 +1386,10 @@ apiDescribe('Database', (persistence: boolean) => { it('WriteBatch.set() supports partials with merge', async () => { return withTestDb(persistence, async db => { - const ref = db.doc('postings/post1').withConverter(postConverterMerge); + const ref = db + .collection('posts') + .doc() + .withConverter(postConverterMerge); await ref.set(new Post('walnut', 'author')); const batch = db.batch(); batch.set(ref, { title: 'olive' }, { merge: true }); @@ -1396,7 +1402,10 @@ apiDescribe('Database', (persistence: boolean) => { it('WriteBatch.set() supports partials with mergeFields', async () => { return withTestDb(persistence, async db => { - const ref = db.doc('postings/post1').withConverter(postConverterMerge); + const ref = db + .collection('posts') + .doc() + .withConverter(postConverterMerge); await ref.set(new Post('walnut', 'author')); const batch = db.batch(); batch.set( @@ -1413,7 +1422,10 @@ apiDescribe('Database', (persistence: boolean) => { it('Transaction.set() supports partials with merge', async () => { return withTestDb(persistence, async db => { - const ref = db.doc('postings/post1').withConverter(postConverterMerge); + const ref = db + .collection('posts') + .doc() + .withConverter(postConverterMerge); await ref.set(new Post('walnut', 'author')); await db.runTransaction(async tx => { tx.set(ref, { title: 'olive' }, { merge: true }); @@ -1426,7 +1438,10 @@ apiDescribe('Database', (persistence: boolean) => { it('Transaction.set() supports partials with mergeFields', async () => { return withTestDb(persistence, async db => { - const ref = db.doc('postings/post1').withConverter(postConverterMerge); + const ref = db + .collection('posts') + .doc() + .withConverter(postConverterMerge); await ref.set(new Post('walnut', 'author')); await db.runTransaction(async tx => { tx.set( @@ -1443,7 +1458,10 @@ apiDescribe('Database', (persistence: boolean) => { it('DocumentReference.set() supports partials with merge', async () => { return withTestDb(persistence, async db => { - const ref = db.doc('postings/post1').withConverter(postConverterMerge); + const ref = db + .collection('posts') + .doc() + .withConverter(postConverterMerge); await ref.set(new Post('walnut', 'author')); await ref.set({ title: 'olive' }, { merge: true }); const doc = await ref.get(); @@ -1454,7 +1472,10 @@ apiDescribe('Database', (persistence: boolean) => { it('DocumentReference.set() supports partials with mergeFields', async () => { return withTestDb(persistence, async db => { - const ref = db.doc('postings/post1').withConverter(postConverterMerge); + const ref = db + .collection('posts') + .doc() + .withConverter(postConverterMerge); await ref.set(new Post('walnut', 'author')); await ref.set( { title: 'olive', author: 'writer' }, From 390d49ccb293beb7abae32d2f2164e81d11957dc Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 24 Jun 2020 11:52:10 -0700 Subject: [PATCH 04/18] delete extra d.ts methods --- packages/firebase/index.d.ts | 11 ----------- packages/firestore-types/index.d.ts | 11 ----------- 2 files changed, 22 deletions(-) diff --git a/packages/firebase/index.d.ts b/packages/firebase/index.d.ts index 65f1d06fa2a..4700613b06f 100644 --- a/packages/firebase/index.d.ts +++ b/packages/firebase/index.d.ts @@ -8333,17 +8333,6 @@ declare namespace firebase.firestore { */ set(documentRef: DocumentReference, data: T): Transaction; - /** - * Writes to the document referred to by the provided `DocumentReference`. - * If the document does not exist yet, it will be created. If you pass - * `SetOptions`, the provided data can be merged into the existing document. - * - * @param documentRef A reference to the document to be set. - * @param data An object of the fields and values for the document. - * @param options An object to configure the set behavior. - * @return This `Transaction` instance. Used for chaining method calls. - */ - /** * Updates fields in the document referred to by the provided * `DocumentReference`. The update will fail if applied to a document that diff --git a/packages/firestore-types/index.d.ts b/packages/firestore-types/index.d.ts index d4f3b84c106..7563427fdc4 100644 --- a/packages/firestore-types/index.d.ts +++ b/packages/firestore-types/index.d.ts @@ -155,11 +155,6 @@ export class Transaction { options: SetOptions ): Transaction; set(documentRef: DocumentReference, data: T): Transaction; - set( - documentRef: DocumentReference, - data: T | Partial, - options?: SetOptions - ): Transaction; update(documentRef: DocumentReference, data: UpdateData): Transaction; update( @@ -181,11 +176,6 @@ export class WriteBatch { options: SetOptions ): WriteBatch; set(documentRef: DocumentReference, data: T): WriteBatch; - set( - documentRef: DocumentReference, - data: T | Partial, - options?: SetOptions - ): WriteBatch; update(documentRef: DocumentReference, data: UpdateData): WriteBatch; update( @@ -227,7 +217,6 @@ export class DocumentReference { set(data: Partial, options: SetOptions): Promise; set(data: T): Promise; - set(data: T | Partial, options?: SetOptions): Promise; update(data: UpdateData): Promise; update( From 4e8ca3ca87f9803f568a12e9e2a151992c1dda66 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 24 Jun 2020 19:32:35 -0700 Subject: [PATCH 05/18] go back to overloads instead of union --- packages/firebase/index.d.ts | 16 ++++++++-------- packages/firestore-types/index.d.ts | 12 ++++-------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/packages/firebase/index.d.ts b/packages/firebase/index.d.ts index 4700613b06f..72b13e50754 100644 --- a/packages/firebase/index.d.ts +++ b/packages/firebase/index.d.ts @@ -15,7 +15,11 @@ * limitations under the License. */ -import { DocumentData } from '@firebase/firestore-types'; +import { + DocumentData, + QueryDocumentSnapshot, + SetOptions +} from '@firebase/firestore-types'; /** * firebase is a global namespace from which all Firebase @@ -7864,9 +7868,8 @@ declare namespace firebase.firestore { * Firestore database). To use set() with `merge` and `mergeFields, * toFirestore() must be defined with `Partial`. */ - toFirestore: - | ((modelObject: T) => DocumentData) - | ((modelObject: Partial, options?: SetOptions) => DocumentData); + toFirestore(modelObject: T): DocumentData; + toFirestore(modelObject: Partial, options: SetOptions): DocumentData; /** * Called by the Firestore SDK to convert Firestore data into an object of @@ -7875,10 +7878,7 @@ declare namespace firebase.firestore { * @param snapshot A QueryDocumentSnapshot containing your data and metadata. * @param options The SnapshotOptions from the initial call to `data()`. */ - fromFirestore: ( - snapshot: QueryDocumentSnapshot, - options: SnapshotOptions - ) => T; + fromFirestore(snapshot: QueryDocumentSnapshot, options: SnapshotOptions): T; } /** diff --git a/packages/firestore-types/index.d.ts b/packages/firestore-types/index.d.ts index 7563427fdc4..2346aebf2eb 100644 --- a/packages/firestore-types/index.d.ts +++ b/packages/firestore-types/index.d.ts @@ -49,14 +49,10 @@ export type LogLevel = export function setLogLevel(logLevel: LogLevel): void; export interface FirestoreDataConverter { - toFirestore: - | ((modelObject: T) => DocumentData) - | ((modelObject: Partial, options?: SetOptions) => DocumentData); - - fromFirestore: ( - snapshot: QueryDocumentSnapshot, - options: SnapshotOptions - ) => T; + toFirestore(modelObject: T): DocumentData; + toFirestore(modelObject: Partial, options: SetOptions): DocumentData; + + fromFirestore(snapshot: QueryDocumentSnapshot, options: SnapshotOptions): T; } export class FirebaseFirestore { From fd11cd7bc7a86793db1a478b7c84cad37ebf46be Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Wed, 24 Jun 2020 19:33:38 -0700 Subject: [PATCH 06/18] typo --- packages/firebase/index.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firebase/index.d.ts b/packages/firebase/index.d.ts index 72b13e50754..1ab92891f9c 100644 --- a/packages/firebase/index.d.ts +++ b/packages/firebase/index.d.ts @@ -7865,7 +7865,7 @@ declare namespace firebase.firestore { /** * Called by the Firestore SDK to convert a custom model object of type T * into a plain Javascript object (suitable for writing directly to the - * Firestore database). To use set() with `merge` and `mergeFields, + * Firestore database). To use set() with `merge` and `mergeFields`, * toFirestore() must be defined with `Partial`. */ toFirestore(modelObject: T): DocumentData; From 767c9363188f646e3c458d6713ea12572cba2e8b Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 25 Jun 2020 10:15:54 -0700 Subject: [PATCH 07/18] fix test --- packages/firestore/test/integration/api/database.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 99e0c07e89b..9cb655b1947 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1372,14 +1372,16 @@ apiDescribe('Database', (persistence: boolean) => { return withTestDb(persistence, async db => { const ref = db .collection('posts') - .doc() + .doc('some-post') .withConverter(postConverter); await ref.set(new Post('walnut', 'author')); const batch = db.batch(); expect(() => batch.set(ref, { title: 'olive' }, { merge: true }) ).to.throw( - 'Function toFirestore() in WriteBatch.set() called with invalid data. Unsupported field value: undefined (found in field author)' + 'Function toFirestore() in WriteBatch.set() called with invalid ' + + 'data. Unsupported field value: undefined (found in field author ' + + 'in document posts/some-post)' ); }); }); From 89f2c8d5096bd0997eb37fa346f76deca693ee3d Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 25 Jun 2020 12:13:12 -0700 Subject: [PATCH 08/18] add changeset, remove changelog --- .changeset/rare-rabbits-grab.md | 6 ++++++ packages/firestore/CHANGELOG.md | 2 -- 2 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 .changeset/rare-rabbits-grab.md diff --git a/.changeset/rare-rabbits-grab.md b/.changeset/rare-rabbits-grab.md new file mode 100644 index 00000000000..b7153bb5e3f --- /dev/null +++ b/.changeset/rare-rabbits-grab.md @@ -0,0 +1,6 @@ +--- +'@firebase/firestore': minor +'@firebase/firestore-types': minor +--- + +Added support for `set()` with merge options when using `FirestoreDataConverter`. diff --git a/packages/firestore/CHANGELOG.md b/packages/firestore/CHANGELOG.md index 0553fb109c3..c9aa89edfdb 100644 --- a/packages/firestore/CHANGELOG.md +++ b/packages/firestore/CHANGELOG.md @@ -1,8 +1,6 @@ # Unreleased - [fixed] Fixed an issue that may have prevented the client from connecting to the backend immediately after a user signed in. -- [fixed] Added support for using `set()` with merge options when using - `FirestoreDataConverter`. # Released - [changed] All known failure cases for Indexed-related crashes have now been From a09f5cf104a8d2e95589891f14cfa689460dd939 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 25 Jun 2020 12:30:39 -0700 Subject: [PATCH 09/18] update changeset to include firebase --- .changeset/{rare-rabbits-grab.md => quiet-coats-type.md} | 1 + 1 file changed, 1 insertion(+) rename .changeset/{rare-rabbits-grab.md => quiet-coats-type.md} (89%) diff --git a/.changeset/rare-rabbits-grab.md b/.changeset/quiet-coats-type.md similarity index 89% rename from .changeset/rare-rabbits-grab.md rename to .changeset/quiet-coats-type.md index b7153bb5e3f..4dd43f683b5 100644 --- a/.changeset/rare-rabbits-grab.md +++ b/.changeset/quiet-coats-type.md @@ -1,4 +1,5 @@ --- +'firebase': minor '@firebase/firestore': minor '@firebase/firestore-types': minor --- From f289a76d067cbb81404af17ec461bee02a7ae742 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 25 Jun 2020 21:40:19 -0700 Subject: [PATCH 10/18] propagate converter into error message --- packages/firestore/lite/src/api/reference.ts | 20 ++++++------- .../firestore/lite/src/api/transaction.ts | 9 ++---- .../firestore/lite/src/api/write_batch.ts | 9 ++---- packages/firestore/src/api/database.ts | 30 +++++++++---------- .../firestore/src/api/user_data_reader.ts | 30 ++++++++++++++----- .../test/integration/api/database.test.ts | 6 ++-- 6 files changed, 55 insertions(+), 49 deletions(-) diff --git a/packages/firestore/lite/src/api/reference.ts b/packages/firestore/lite/src/api/reference.ts index dc6736ee66c..015529455ad 100644 --- a/packages/firestore/lite/src/api/reference.ts +++ b/packages/firestore/lite/src/api/reference.ts @@ -456,11 +456,7 @@ export function setDoc( ): Promise { const ref = cast(reference, DocumentReference); - const [convertedValue] = applyFirestoreDataConverter( - ref._converter, - data, - 'setDoc' - ); + const convertedValue = applyFirestoreDataConverter(ref._converter, data); const dataReader = newUserDataReader(ref.firestore); const parsed = dataReader.parseSetData( 'setDoc', @@ -548,14 +544,16 @@ export function addDoc( const collRef = cast(reference, CollectionReference); const docRef = doc(collRef); - const [convertedValue] = applyFirestoreDataConverter( - collRef._converter, - data, - 'addDoc' - ); + const convertedValue = applyFirestoreDataConverter(collRef._converter, data); const dataReader = newUserDataReader(collRef.firestore); - const parsed = dataReader.parseSetData('addDoc', docRef._key, convertedValue); + const parsed = dataReader.parseSetData( + 'addDoc', + docRef._key, + convertedValue, + {}, + docRef._converter !== null + ); return collRef.firestore ._getDatastore() diff --git a/packages/firestore/lite/src/api/transaction.ts b/packages/firestore/lite/src/api/transaction.ts index 27fa86cf7ea..3b966579f86 100644 --- a/packages/firestore/lite/src/api/transaction.ts +++ b/packages/firestore/lite/src/api/transaction.ts @@ -95,16 +95,13 @@ export class Transaction implements firestore.Transaction { options?: firestore.SetOptions ): Transaction { const ref = validateReference(documentRef, this._firestore); - const [convertedValue] = applyFirestoreDataConverter( - ref._converter, - value, - 'Transaction.set' - ); + const convertedValue = applyFirestoreDataConverter(ref._converter, value); const parsed = this._dataReader.parseSetData( 'Transaction.set', ref._key, convertedValue, - options + options, + ref._converter !== null ); this._transaction.set(ref._key, parsed); return this; diff --git a/packages/firestore/lite/src/api/write_batch.ts b/packages/firestore/lite/src/api/write_batch.ts index b190ca9d1c2..db418889c21 100644 --- a/packages/firestore/lite/src/api/write_batch.ts +++ b/packages/firestore/lite/src/api/write_batch.ts @@ -59,17 +59,14 @@ export class WriteBatch implements firestore.WriteBatch { this.verifyNotCommitted(); const ref = validateReference(documentRef, this._firestore); - const [convertedValue] = applyFirestoreDataConverter( - ref._converter, - value, - 'WriteBatch.set' - ); + const convertedValue = applyFirestoreDataConverter(ref._converter, value); const parsed = this._dataReader.parseSetData( 'WriteBatch.set', ref._key, convertedValue, - options + options, + ref._converter !== null ); this._mutations = this._mutations.concat( parsed.toMutations(ref._key, Precondition.none()) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index a6e2fed59bb..8a2d39ebab2 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -744,17 +744,17 @@ export class Transaction implements firestore.Transaction { this._firestore ); options = validateSetOptions('Transaction.set', options); - const [convertedValue, functionName] = applyFirestoreDataConverter( + const convertedValue = applyFirestoreDataConverter( ref._converter, value, - 'Transaction.set', options ); const parsed = this._firestore._dataReader.parseSetData( - functionName, + 'Transaction.set', ref._key, convertedValue, - options + options, + ref._converter !== null ); this._transaction.set(ref._key, parsed); return this; @@ -851,17 +851,17 @@ export class WriteBatch implements firestore.WriteBatch { this._firestore ); options = validateSetOptions('WriteBatch.set', options); - const [convertedValue, functionName] = applyFirestoreDataConverter( + const convertedValue = applyFirestoreDataConverter( ref._converter, value, - 'WriteBatch.set', options ); const parsed = this._firestore._dataReader.parseSetData( - functionName, + 'WriteBatch.set', ref._key, convertedValue, - options + options, + ref._converter !== null ); this._mutations = this._mutations.concat( parsed.toMutations(ref._key, Precondition.none()) @@ -1051,17 +1051,17 @@ export class DocumentReference set(value: T | Partial, options?: firestore.SetOptions): Promise { validateBetweenNumberOfArgs('DocumentReference.set', arguments, 1, 2); options = validateSetOptions('DocumentReference.set', options); - const [convertedValue, functionName] = applyFirestoreDataConverter( + const convertedValue = applyFirestoreDataConverter( this._converter, value, - 'DocumentReference.set', options ); const parsed = this.firestore._dataReader.parseSetData( - functionName, + 'DocumentReference.set', this._key, convertedValue, - options + options, + this._converter !== null ); return this._firestoreClient.write( parsed.toMutations(this._key, Precondition.none()) @@ -2597,9 +2597,8 @@ function resultChangeType(type: ChangeType): firestore.DocumentChangeType { export function applyFirestoreDataConverter( converter: UntypedFirestoreDataConverter | null, value: T, - functionName: string, options?: firestore.SetOptions -): [firestore.DocumentData, string] { +): firestore.DocumentData { let convertedValue; if (converter) { if (options && (options.merge || options.mergeFields)) { @@ -2610,11 +2609,10 @@ export function applyFirestoreDataConverter( } else { convertedValue = converter.toFirestore(value); } - functionName = 'toFirestore() in ' + functionName; } else { convertedValue = value as firestore.DocumentData; } - return [convertedValue, functionName]; + return convertedValue; } function contains(obj: object, key: string): obj is { key: unknown } { diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index bb50802efd8..711c6505d42 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -173,6 +173,11 @@ interface ContextSettings { * If not set, elements are treated as if they were outside of arrays. */ readonly arrayElement?: boolean; + /** + * Whether or not a converter was specified in this context. If set, error + * messages will reference the converter when invalid data is provided. + */ + readonly hasConverter?: boolean; } /** A "context" object passed around while parsing user data. */ @@ -259,7 +264,8 @@ export class ParseContext { reason, this.settings.methodName, this.path, - this.settings.targetDoc + this.settings.targetDoc, + this.settings.hasConverter ); } @@ -314,14 +320,16 @@ export class UserDataReader { methodName: string, targetDoc: DocumentKey, input: unknown, - options: firestore.SetOptions = {} + options: firestore.SetOptions = {}, + hasConverter: boolean ): ParsedSetData { const context = this.createContext( options.merge || options.mergeFields ? UserDataSource.MergeSet : UserDataSource.Set, methodName, - targetDoc + targetDoc, + hasConverter ); validatePlainObject('Data must be an object, but it was:', context, input); const updateData = parseObject(input, context)!; @@ -494,7 +502,8 @@ export class UserDataReader { private createContext( dataSource: UserDataSource, methodName: string, - targetDoc?: DocumentKey + targetDoc?: DocumentKey, + hasConverter = false ): ParseContext { return new ParseContext( { @@ -502,7 +511,8 @@ export class UserDataReader { methodName, targetDoc, path: FieldPath.emptyPath(), - arrayElement: false + arrayElement: false, + hasConverter }, this.databaseId, this.serializer, @@ -803,10 +813,16 @@ function createError( reason: string, methodName: string, path?: FieldPath, - targetDoc?: DocumentKey + targetDoc?: DocumentKey, + hasConverter = false ): Error { const hasPath = path && !path.isEmpty(); const hasDocument = targetDoc !== undefined; + let message = `Function ${methodName}() called with invalid data`; + if (hasConverter) { + message += ' (via `toFirestore()`)'; + } + message += '. '; let description = ''; if (hasPath || hasDocument) { @@ -823,7 +839,7 @@ function createError( return new FirestoreError( Code.INVALID_ARGUMENT, - `Function ${methodName}() called with invalid data. ` + reason + description + message + reason + description ); } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 9cb655b1947..097579d9dfd 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -1379,9 +1379,9 @@ apiDescribe('Database', (persistence: boolean) => { expect(() => batch.set(ref, { title: 'olive' }, { merge: true }) ).to.throw( - 'Function toFirestore() in WriteBatch.set() called with invalid ' + - 'data. Unsupported field value: undefined (found in field author ' + - 'in document posts/some-post)' + 'Function WriteBatch.set() called with invalid ' + + 'data (via `toFirestore()`). Unsupported field value: undefined ' + + '(found in field author in document posts/some-post)' ); }); }); From 695d0a45b2d2ae70a55c7eb453cf07a771b25d0a Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 25 Jun 2020 21:43:05 -0700 Subject: [PATCH 11/18] remove unneeded import --- packages/firebase/index.d.ts | 6 ------ packages/firestore/lite/src/api/reference.ts | 3 ++- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/firebase/index.d.ts b/packages/firebase/index.d.ts index 1ab92891f9c..c34d72dad5f 100644 --- a/packages/firebase/index.d.ts +++ b/packages/firebase/index.d.ts @@ -15,12 +15,6 @@ * limitations under the License. */ -import { - DocumentData, - QueryDocumentSnapshot, - SetOptions -} from '@firebase/firestore-types'; - /** * firebase is a global namespace from which all Firebase * services are accessed. diff --git a/packages/firestore/lite/src/api/reference.ts b/packages/firestore/lite/src/api/reference.ts index 015529455ad..6b5760eb059 100644 --- a/packages/firestore/lite/src/api/reference.ts +++ b/packages/firestore/lite/src/api/reference.ts @@ -462,7 +462,8 @@ export function setDoc( 'setDoc', ref._key, convertedValue, - options + options, + ref._converter !== null ); return ref.firestore From 3c00f9617765d6527488725527de921f5f9275e1 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 25 Jun 2020 23:12:14 -0700 Subject: [PATCH 12/18] change param order --- packages/firestore/lite/src/api/reference.ts | 8 ++--- .../firestore/lite/src/api/transaction.ts | 4 +-- .../firestore/lite/src/api/write_batch.ts | 4 +-- packages/firestore/src/api/database.ts | 12 ++++---- .../firestore/src/api/user_data_reader.ts | 30 +++++++++++++------ 5 files changed, 35 insertions(+), 23 deletions(-) diff --git a/packages/firestore/lite/src/api/reference.ts b/packages/firestore/lite/src/api/reference.ts index 6b5760eb059..fc48f56fb2b 100644 --- a/packages/firestore/lite/src/api/reference.ts +++ b/packages/firestore/lite/src/api/reference.ts @@ -462,8 +462,8 @@ export function setDoc( 'setDoc', ref._key, convertedValue, - options, - ref._converter !== null + ref._converter !== null, + options ); return ref.firestore @@ -552,8 +552,8 @@ export function addDoc( 'addDoc', docRef._key, convertedValue, - {}, - docRef._converter !== null + docRef._converter !== null, + {} ); return collRef.firestore diff --git a/packages/firestore/lite/src/api/transaction.ts b/packages/firestore/lite/src/api/transaction.ts index 3b966579f86..dcbe0c34eb3 100644 --- a/packages/firestore/lite/src/api/transaction.ts +++ b/packages/firestore/lite/src/api/transaction.ts @@ -100,8 +100,8 @@ export class Transaction implements firestore.Transaction { 'Transaction.set', ref._key, convertedValue, - options, - ref._converter !== null + ref._converter !== null, + options ); this._transaction.set(ref._key, parsed); return this; diff --git a/packages/firestore/lite/src/api/write_batch.ts b/packages/firestore/lite/src/api/write_batch.ts index db418889c21..f012df989ee 100644 --- a/packages/firestore/lite/src/api/write_batch.ts +++ b/packages/firestore/lite/src/api/write_batch.ts @@ -65,8 +65,8 @@ export class WriteBatch implements firestore.WriteBatch { 'WriteBatch.set', ref._key, convertedValue, - options, - ref._converter !== null + ref._converter !== null, + options ); this._mutations = this._mutations.concat( parsed.toMutations(ref._key, Precondition.none()) diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 8a2d39ebab2..ecc9656738f 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -753,8 +753,8 @@ export class Transaction implements firestore.Transaction { 'Transaction.set', ref._key, convertedValue, - options, - ref._converter !== null + ref._converter !== null, + options ); this._transaction.set(ref._key, parsed); return this; @@ -860,8 +860,8 @@ export class WriteBatch implements firestore.WriteBatch { 'WriteBatch.set', ref._key, convertedValue, - options, - ref._converter !== null + ref._converter !== null, + options ); this._mutations = this._mutations.concat( parsed.toMutations(ref._key, Precondition.none()) @@ -1060,8 +1060,8 @@ export class DocumentReference 'DocumentReference.set', this._key, convertedValue, - options, - this._converter !== null + this._converter !== null, + options ); return this._firestoreClient.write( parsed.toMutations(this._key, Precondition.none()) diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index 711c6505d42..24d44886315 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -174,7 +174,7 @@ interface ContextSettings { */ readonly arrayElement?: boolean; /** - * Whether or not a converter was specified in this context. If set, error + * Whether or not a converter was specified in this context. If true, error * messages will reference the converter when invalid data is provided. */ readonly hasConverter?: boolean; @@ -263,9 +263,9 @@ export class ParseContext { return createError( reason, this.settings.methodName, + this.settings.hasConverter || false, this.path, - this.settings.targetDoc, - this.settings.hasConverter + this.settings.targetDoc ); } @@ -320,8 +320,8 @@ export class UserDataReader { methodName: string, targetDoc: DocumentKey, input: unknown, - options: firestore.SetOptions = {}, - hasConverter: boolean + hasConverter: boolean, + options: firestore.SetOptions = {} ): ParsedSetData { const context = this.createContext( options.merge || options.mergeFields @@ -784,7 +784,13 @@ export function fieldPathFromArgument( return fieldPathFromDotSeparatedString(methodName, path); } else { const message = 'Field path arguments must be of type string or FieldPath.'; - throw createError(message, methodName, undefined, targetDoc); + throw createError( + message, + methodName, + /* hasConverter= */ false, + /* path= */ undefined, + targetDoc + ); } } @@ -805,16 +811,22 @@ export function fieldPathFromDotSeparatedString( return fromDotSeparatedString(path)._internalPath; } catch (e) { const message = errorMessage(e); - throw createError(message, methodName, undefined, targetDoc); + throw createError( + message, + methodName, + /* hasConverter= */ false, + /* path= */ undefined, + targetDoc + ); } } function createError( reason: string, methodName: string, + hasConverter: boolean, path?: FieldPath, - targetDoc?: DocumentKey, - hasConverter = false + targetDoc?: DocumentKey ): Error { const hasPath = path && !path.isEmpty(); const hasDocument = targetDoc !== undefined; From 57ed84b8a10c65828d308fe389e7b001a6ec79d1 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Fri, 26 Jun 2020 12:09:24 -0700 Subject: [PATCH 13/18] add backticks --- packages/firebase/index.d.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firebase/index.d.ts b/packages/firebase/index.d.ts index c34d72dad5f..518f22d130a 100644 --- a/packages/firebase/index.d.ts +++ b/packages/firebase/index.d.ts @@ -7859,8 +7859,8 @@ declare namespace firebase.firestore { /** * Called by the Firestore SDK to convert a custom model object of type T * into a plain Javascript object (suitable for writing directly to the - * Firestore database). To use set() with `merge` and `mergeFields`, - * toFirestore() must be defined with `Partial`. + * Firestore database). To use `set()` with `merge` and `mergeFields`, + * `toFirestore()` must be defined with `Partial`. */ toFirestore(modelObject: T): DocumentData; toFirestore(modelObject: Partial, options: SetOptions): DocumentData; From 6e0beb91cd230052b04fb653747597abfa109d81 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 25 Jun 2020 21:46:05 -0700 Subject: [PATCH 14/18] add set() overrides to lite sdk --- packages/firestore/lite/index.d.ts | 8 +- .../firestore/lite/src/api/write_batch.ts | 6 +- .../firestore/lite/test/integration.test.ts | 141 +++++++++++++++++- .../firestore/src/api/user_data_reader.ts | 4 + 4 files changed, 152 insertions(+), 7 deletions(-) diff --git a/packages/firestore/lite/index.d.ts b/packages/firestore/lite/index.d.ts index 0cf2a0c25e6..84104122ae5 100644 --- a/packages/firestore/lite/index.d.ts +++ b/packages/firestore/lite/index.d.ts @@ -45,6 +45,7 @@ export function setLogLevel(logLevel: LogLevel): void; export interface FirestoreDataConverter { toFirestore(modelObject: T): DocumentData; + toFirestore(modelObject: Partial, options: SetOptions): DocumentData; fromFirestore(snapshot: QueryDocumentSnapshot): T; } @@ -182,9 +183,10 @@ export class WriteBatch { commit(): Promise; } -export type SetOptions = - | { merge: true } - | { mergeFields: Array }; +export interface SetOptions { + readonly merge?: boolean; + readonly mergeFields?: Array; +} export class DocumentReference { private constructor(); diff --git a/packages/firestore/lite/src/api/write_batch.ts b/packages/firestore/lite/src/api/write_batch.ts index f012df989ee..7a5a818de36 100644 --- a/packages/firestore/lite/src/api/write_batch.ts +++ b/packages/firestore/lite/src/api/write_batch.ts @@ -60,7 +60,6 @@ export class WriteBatch implements firestore.WriteBatch { const ref = validateReference(documentRef, this._firestore); const convertedValue = applyFirestoreDataConverter(ref._converter, value); - const parsed = this._dataReader.parseSetData( 'WriteBatch.set', ref._key, @@ -162,7 +161,10 @@ export function validateReference( 'Provided document reference is from a different Firestore instance.' ); } else { - return cast(documentRef, DocumentReference) as DocumentReference; + return (cast( + documentRef, + DocumentReference + ) as unknown) as DocumentReference; } } diff --git a/packages/firestore/lite/test/integration.test.ts b/packages/firestore/lite/test/integration.test.ts index 18172ab0f30..c7c4ab1e5f2 100644 --- a/packages/firestore/lite/test/integration.test.ts +++ b/packages/firestore/lite/test/integration.test.ts @@ -881,7 +881,8 @@ describe('equality', () => { expect(refEqual(coll1a, coll2)).to.be.false; const coll1c = collection(firestore, 'a').withConverter({ - toFirestore: data => data as firestore.DocumentData, + toFirestore: (data: firestore.DocumentData) => + data as firestore.DocumentData, fromFirestore: snap => snap.data() }); expect(refEqual(coll1a, coll1c)).to.be.false; @@ -900,7 +901,8 @@ describe('equality', () => { expect(refEqual(doc1a, doc2)).to.be.false; const doc1c = collection(firestore, 'a').withConverter({ - toFirestore: data => data as firestore.DocumentData, + toFirestore: (data: firestore.DocumentData) => + data as firestore.DocumentData, fromFirestore: snap => snap.data() }); expect(refEqual(doc1a, doc1c)).to.be.false; @@ -985,6 +987,35 @@ describe('withConverter() support', () => { } }; + const postConverterMerge = { + toFirestore( + post: Partial, + options?: firestore.SetOptions + ): firestore.DocumentData { + if ( + options && + ((options as { merge: true }).merge || + (options as { mergeFields: Array }).mergeFields) + ) { + expect(post).to.not.be.an.instanceof(Post); + } else { + expect(post).to.be.an.instanceof(Post); + } + const result: firestore.DocumentData = {}; + if (post.title) { + result.title = post.title; + } + if (post.author) { + result.author = post.author; + } + return result; + }, + fromFirestore(snapshot: firestore.QueryDocumentSnapshot): Post { + const data = snapshot.data(); + return new Post(data.title, data.author); + } + }; + it('for DocumentReference.withConverter()', () => { return withTestDoc(async docRef => { docRef = docRef.withConverter(postConverter); @@ -1045,4 +1076,110 @@ describe('withConverter() support', () => { expect(refEqual(docRef, docRef2)).to.be.false; }); }); + + it('requires the correct converter for Partial usage', async () => { + return withTestDb(async db => { + const coll = collection(db, 'posts'); + const ref = doc(coll, 'post').withConverter(postConverter); + const batch = writeBatch(db); + expect(() => + batch.set(ref, { title: 'olive' }, { merge: true }) + ).to.throw( + 'Function WriteBatch.set() called with invalid data. Unsupported ' + + 'field value: undefined (found in field author in document posts/post)' + ); + }); + }); + + it('WriteBatch.set() supports partials with merge', async () => { + return withTestDb(async db => { + const coll = collection(db, 'posts'); + const ref = doc(coll, 'post').withConverter(postConverterMerge); + await setDoc(ref, new Post('walnut', 'author')); + const batch = writeBatch(db); + batch.set(ref, { title: 'olive' }, { merge: true }); + await batch.commit(); + const postDoc = await getDoc(ref); + expect(postDoc.get('title')).to.equal('olive'); + expect(postDoc.get('author')).to.equal('author'); + }); + }); + + it('WriteBatch.set() supports partials with mergeFields', async () => { + return withTestDb(async db => { + const coll = collection(db, 'posts'); + const ref = doc(coll, 'post').withConverter(postConverterMerge); + await setDoc(ref, new Post('walnut', 'author')); + const batch = writeBatch(db); + batch.set(ref, { title: 'olive' }, { merge: true }); + await batch.commit(); + const postDoc = await getDoc(ref); + expect(postDoc.get('title')).to.equal('olive'); + expect(postDoc.get('author')).to.equal('author'); + }); + }); + + it('Transaction.set() supports partials with merge', async () => { + return withTestDb(async db => { + const coll = collection(db, 'posts'); + const ref = doc(coll, 'post').withConverter(postConverterMerge); + await setDoc(ref, new Post('walnut', 'author')); + await runTransaction(db, async tx => { + tx.set( + ref, + { title: 'olive', author: 'person' }, + { mergeFields: ['title'] } + ); + }); + const postDoc = await getDoc(ref); + expect(postDoc.get('title')).to.equal('olive'); + expect(postDoc.get('author')).to.equal('author'); + }); + }); + + it('Transaction.set() supports partials with mergeFields', async () => { + return withTestDb(async db => { + const coll = collection(db, 'posts'); + const ref = doc(coll, 'post').withConverter(postConverterMerge); + await setDoc(ref, new Post('walnut', 'author')); + await runTransaction(db, async tx => { + tx.set( + ref, + { title: 'olive', author: 'person' }, + { mergeFields: ['title'] } + ); + }); + const postDoc = await getDoc(ref); + expect(postDoc.get('title')).to.equal('olive'); + expect(postDoc.get('author')).to.equal('author'); + }); + }); + + it('DocumentReference.set() supports partials with merge', async () => { + return withTestDb(async db => { + const coll = collection(db, 'posts'); + const ref = doc(coll, 'post').withConverter(postConverterMerge); + await setDoc(ref, new Post('walnut', 'author')); + await setDoc(ref, { title: 'olive' }, { merge: true }); + const postDoc = await getDoc(ref); + expect(postDoc.get('title')).to.equal('olive'); + expect(postDoc.get('author')).to.equal('author'); + }); + }); + + it('DocumentReference.set() supports partials with mergeFields', async () => { + return withTestDb(async db => { + const coll = collection(db, 'posts'); + const ref = doc(coll, 'post').withConverter(postConverterMerge); + await setDoc(ref, new Post('walnut', 'author')); + await setDoc( + ref, + { title: 'olive', author: 'writer' }, + { mergeFields: ['title'] } + ); + const postDoc = await getDoc(ref); + expect(postDoc.get('title')).to.equal('olive'); + expect(postDoc.get('author')).to.equal('author'); + }); + }); }); diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index 24d44886315..71abaeb5bec 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -58,6 +58,10 @@ const RESERVED_FIELD_REGEX = /^__.*__$/; */ export interface UntypedFirestoreDataConverter { toFirestore(modelObject: T): firestore.DocumentData; + toFirestore( + modelObject: Partial, + options: firestore.SetOptions + ): firestore.DocumentData; fromFirestore(snapshot: unknown, options?: unknown): T; } From dc4239abcfd2f14338f06db8cc9bcb12f7d19bc0 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 25 Jun 2020 21:49:41 -0700 Subject: [PATCH 15/18] update test --- packages/firestore/lite/src/api/write_batch.ts | 5 +---- packages/firestore/lite/test/integration.test.ts | 5 +++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/firestore/lite/src/api/write_batch.ts b/packages/firestore/lite/src/api/write_batch.ts index 7a5a818de36..a31bf377fbc 100644 --- a/packages/firestore/lite/src/api/write_batch.ts +++ b/packages/firestore/lite/src/api/write_batch.ts @@ -161,10 +161,7 @@ export function validateReference( 'Provided document reference is from a different Firestore instance.' ); } else { - return (cast( - documentRef, - DocumentReference - ) as unknown) as DocumentReference; + return cast(documentRef, DocumentReference) as DocumentReference; } } diff --git a/packages/firestore/lite/test/integration.test.ts b/packages/firestore/lite/test/integration.test.ts index c7c4ab1e5f2..14362731a11 100644 --- a/packages/firestore/lite/test/integration.test.ts +++ b/packages/firestore/lite/test/integration.test.ts @@ -1085,8 +1085,9 @@ describe('withConverter() support', () => { expect(() => batch.set(ref, { title: 'olive' }, { merge: true }) ).to.throw( - 'Function WriteBatch.set() called with invalid data. Unsupported ' + - 'field value: undefined (found in field author in document posts/post)' + 'Function WriteBatch.set() called with invalid data ' + + '(via `toFirestore()`). Unsupported field value: undefined ' + + '(found in field author in document posts/post)' ); }); }); From 699e0e07611e56289837abd695034b5a2177f347 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Fri, 26 Jun 2020 15:44:15 -0700 Subject: [PATCH 16/18] consolidate tests --- packages/firestore/exp/index.d.ts | 10 +- packages/firestore/lite/index.d.ts | 2 +- packages/firestore/lite/src/api/reference.ts | 6 +- .../firestore/lite/src/api/transaction.ts | 6 +- .../firestore/lite/src/api/write_batch.ts | 6 +- packages/firestore/lite/test/helpers.ts | 48 +++++ .../firestore/lite/test/integration.test.ts | 165 +++--------------- 7 files changed, 97 insertions(+), 146 deletions(-) diff --git a/packages/firestore/exp/index.d.ts b/packages/firestore/exp/index.d.ts index 78ae071fa5d..606a236f038 100644 --- a/packages/firestore/exp/index.d.ts +++ b/packages/firestore/exp/index.d.ts @@ -64,7 +64,8 @@ export function setLogLevel(logLevel: LogLevel): void; export interface FirestoreDataConverter { toFirestore(modelObject: T): DocumentData; - fromFirestore(snapshot: QueryDocumentSnapshot): T; + toFirestore(modelObject: Partial, options: SetOptions): DocumentData; + fromFirestore(snapshot: QueryDocumentSnapshot): T; } export class FirebaseFirestore { @@ -217,9 +218,10 @@ export class WriteBatch { commit(): Promise; } -export type SetOptions = - | { merge: true } - | { mergeFields: Array }; +export interface SetOptions { + readonly merge?: boolean; + readonly mergeFields?: Array; +} export class DocumentReference { private constructor(); diff --git a/packages/firestore/lite/index.d.ts b/packages/firestore/lite/index.d.ts index 84104122ae5..b589ed99f51 100644 --- a/packages/firestore/lite/index.d.ts +++ b/packages/firestore/lite/index.d.ts @@ -46,7 +46,7 @@ export function setLogLevel(logLevel: LogLevel): void; export interface FirestoreDataConverter { toFirestore(modelObject: T): DocumentData; toFirestore(modelObject: Partial, options: SetOptions): DocumentData; - fromFirestore(snapshot: QueryDocumentSnapshot): T; + fromFirestore(snapshot: QueryDocumentSnapshot): T; } export class FirebaseFirestore { diff --git a/packages/firestore/lite/src/api/reference.ts b/packages/firestore/lite/src/api/reference.ts index fc48f56fb2b..6b4b09a7b57 100644 --- a/packages/firestore/lite/src/api/reference.ts +++ b/packages/firestore/lite/src/api/reference.ts @@ -456,7 +456,11 @@ export function setDoc( ): Promise { const ref = cast(reference, DocumentReference); - const convertedValue = applyFirestoreDataConverter(ref._converter, data); + const convertedValue = applyFirestoreDataConverter( + ref._converter, + data, + options + ); const dataReader = newUserDataReader(ref.firestore); const parsed = dataReader.parseSetData( 'setDoc', diff --git a/packages/firestore/lite/src/api/transaction.ts b/packages/firestore/lite/src/api/transaction.ts index dcbe0c34eb3..6ee7e60f5ae 100644 --- a/packages/firestore/lite/src/api/transaction.ts +++ b/packages/firestore/lite/src/api/transaction.ts @@ -95,7 +95,11 @@ export class Transaction implements firestore.Transaction { options?: firestore.SetOptions ): Transaction { const ref = validateReference(documentRef, this._firestore); - const convertedValue = applyFirestoreDataConverter(ref._converter, value); + const convertedValue = applyFirestoreDataConverter( + ref._converter, + value, + options + ); const parsed = this._dataReader.parseSetData( 'Transaction.set', ref._key, diff --git a/packages/firestore/lite/src/api/write_batch.ts b/packages/firestore/lite/src/api/write_batch.ts index a31bf377fbc..bf8bab89fea 100644 --- a/packages/firestore/lite/src/api/write_batch.ts +++ b/packages/firestore/lite/src/api/write_batch.ts @@ -59,7 +59,11 @@ export class WriteBatch implements firestore.WriteBatch { this.verifyNotCommitted(); const ref = validateReference(documentRef, this._firestore); - const convertedValue = applyFirestoreDataConverter(ref._converter, value); + const convertedValue = applyFirestoreDataConverter( + ref._converter, + value, + options + ); const parsed = this._dataReader.parseSetData( 'WriteBatch.set', ref._key, diff --git a/packages/firestore/lite/test/helpers.ts b/packages/firestore/lite/test/helpers.ts index 953be964d42..713c3145862 100644 --- a/packages/firestore/lite/test/helpers.ts +++ b/packages/firestore/lite/test/helpers.ts @@ -26,6 +26,7 @@ import { DEFAULT_SETTINGS } from '../../test/integration/util/settings'; import { AutoId } from '../../src/util/misc'; +import { expect } from 'chai'; let appCount = 0; @@ -89,3 +90,50 @@ export function withTestCollection( return fn(collection(db, AutoId.newId())); }); } + +// Used for testing the FirestoreDataConverter. +export class Post { + constructor(readonly title: string, readonly author: string) {} + byline(): string { + return this.title + ', by ' + this.author; + } +} + +export const postConverter = { + toFirestore(post: Post): firestore.DocumentData { + return { title: post.title, author: post.author }; + }, + fromFirestore(snapshot: firestore.QueryDocumentSnapshot): Post { + const data = snapshot.data(); + return new Post(data.title, data.author); + } +}; + +export const postConverterMerge = { + toFirestore( + post: Partial, + options?: firestore.SetOptions + ): firestore.DocumentData { + if ( + options && + ((options as { merge: true }).merge || + (options as { mergeFields: Array }).mergeFields) + ) { + expect(post).to.not.be.an.instanceof(Post); + } else { + expect(post).to.be.an.instanceof(Post); + } + const result: firestore.DocumentData = {}; + if (post.title) { + result.title = post.title; + } + if (post.author) { + result.author = post.author; + } + return result; + }, + fromFirestore(snapshot: firestore.QueryDocumentSnapshot): Post { + const data = snapshot.data(); + return new Post(data.title, data.author); + } +}; diff --git a/packages/firestore/lite/test/integration.test.ts b/packages/firestore/lite/test/integration.test.ts index 14362731a11..ec0182ad55b 100644 --- a/packages/firestore/lite/test/integration.test.ts +++ b/packages/firestore/lite/test/integration.test.ts @@ -28,6 +28,9 @@ import { terminate } from '../src/api/database'; import { + Post, + postConverter, + postConverterMerge, withTestCollection, withTestCollectionAndInitialData, withTestDb, @@ -480,6 +483,30 @@ function genericMutationTests( }); }); + it('supports partials with merge', async () => { + return withTestDb(async db => { + const coll = collection(db, 'posts'); + const ref = doc(coll, 'post').withConverter(postConverterMerge); + await setDoc(ref, new Post('walnut', 'author')); + await setDoc(ref, { title: 'olive' }, { merge: true }); + const postDoc = await getDoc(ref); + expect(postDoc.get('title')).to.equal('olive'); + expect(postDoc.get('author')).to.equal('author'); + }); + }); + + it('supports partials with mergeFields', async () => { + return withTestDb(async db => { + const coll = collection(db, 'posts'); + const ref = doc(coll, 'post').withConverter(postConverterMerge); + await setDoc(ref, new Post('walnut', 'author')); + await setDoc(ref, { title: 'olive' }, { mergeFields: ['title'] }); + const postDoc = await getDoc(ref); + expect(postDoc.get('title')).to.equal('olive'); + expect(postDoc.get('author')).to.equal('author'); + }); + }); + it('throws when user input fails validation', () => { return withTestDoc(async docRef => { if (validationUsesPromises) { @@ -970,52 +997,6 @@ describe('equality', () => { }); describe('withConverter() support', () => { - class Post { - constructor(readonly title: string, readonly author: string) {} - byline(): string { - return this.title + ', by ' + this.author; - } - } - - const postConverter = { - toFirestore(post: Post): firestore.DocumentData { - return { title: post.title, author: post.author }; - }, - fromFirestore(snapshot: firestore.QueryDocumentSnapshot): Post { - const data = snapshot.data(); - return new Post(data.title, data.author); - } - }; - - const postConverterMerge = { - toFirestore( - post: Partial, - options?: firestore.SetOptions - ): firestore.DocumentData { - if ( - options && - ((options as { merge: true }).merge || - (options as { mergeFields: Array }).mergeFields) - ) { - expect(post).to.not.be.an.instanceof(Post); - } else { - expect(post).to.be.an.instanceof(Post); - } - const result: firestore.DocumentData = {}; - if (post.title) { - result.title = post.title; - } - if (post.author) { - result.author = post.author; - } - return result; - }, - fromFirestore(snapshot: firestore.QueryDocumentSnapshot): Post { - const data = snapshot.data(); - return new Post(data.title, data.author); - } - }; - it('for DocumentReference.withConverter()', () => { return withTestDoc(async docRef => { docRef = docRef.withConverter(postConverter); @@ -1091,96 +1072,4 @@ describe('withConverter() support', () => { ); }); }); - - it('WriteBatch.set() supports partials with merge', async () => { - return withTestDb(async db => { - const coll = collection(db, 'posts'); - const ref = doc(coll, 'post').withConverter(postConverterMerge); - await setDoc(ref, new Post('walnut', 'author')); - const batch = writeBatch(db); - batch.set(ref, { title: 'olive' }, { merge: true }); - await batch.commit(); - const postDoc = await getDoc(ref); - expect(postDoc.get('title')).to.equal('olive'); - expect(postDoc.get('author')).to.equal('author'); - }); - }); - - it('WriteBatch.set() supports partials with mergeFields', async () => { - return withTestDb(async db => { - const coll = collection(db, 'posts'); - const ref = doc(coll, 'post').withConverter(postConverterMerge); - await setDoc(ref, new Post('walnut', 'author')); - const batch = writeBatch(db); - batch.set(ref, { title: 'olive' }, { merge: true }); - await batch.commit(); - const postDoc = await getDoc(ref); - expect(postDoc.get('title')).to.equal('olive'); - expect(postDoc.get('author')).to.equal('author'); - }); - }); - - it('Transaction.set() supports partials with merge', async () => { - return withTestDb(async db => { - const coll = collection(db, 'posts'); - const ref = doc(coll, 'post').withConverter(postConverterMerge); - await setDoc(ref, new Post('walnut', 'author')); - await runTransaction(db, async tx => { - tx.set( - ref, - { title: 'olive', author: 'person' }, - { mergeFields: ['title'] } - ); - }); - const postDoc = await getDoc(ref); - expect(postDoc.get('title')).to.equal('olive'); - expect(postDoc.get('author')).to.equal('author'); - }); - }); - - it('Transaction.set() supports partials with mergeFields', async () => { - return withTestDb(async db => { - const coll = collection(db, 'posts'); - const ref = doc(coll, 'post').withConverter(postConverterMerge); - await setDoc(ref, new Post('walnut', 'author')); - await runTransaction(db, async tx => { - tx.set( - ref, - { title: 'olive', author: 'person' }, - { mergeFields: ['title'] } - ); - }); - const postDoc = await getDoc(ref); - expect(postDoc.get('title')).to.equal('olive'); - expect(postDoc.get('author')).to.equal('author'); - }); - }); - - it('DocumentReference.set() supports partials with merge', async () => { - return withTestDb(async db => { - const coll = collection(db, 'posts'); - const ref = doc(coll, 'post').withConverter(postConverterMerge); - await setDoc(ref, new Post('walnut', 'author')); - await setDoc(ref, { title: 'olive' }, { merge: true }); - const postDoc = await getDoc(ref); - expect(postDoc.get('title')).to.equal('olive'); - expect(postDoc.get('author')).to.equal('author'); - }); - }); - - it('DocumentReference.set() supports partials with mergeFields', async () => { - return withTestDb(async db => { - const coll = collection(db, 'posts'); - const ref = doc(coll, 'post').withConverter(postConverterMerge); - await setDoc(ref, new Post('walnut', 'author')); - await setDoc( - ref, - { title: 'olive', author: 'writer' }, - { mergeFields: ['title'] } - ); - const postDoc = await getDoc(ref); - expect(postDoc.get('title')).to.equal('olive'); - expect(postDoc.get('author')).to.equal('author'); - }); - }); }); From fdd50728cad23765c22ea3e0572337a468031b88 Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Fri, 26 Jun 2020 15:48:20 -0700 Subject: [PATCH 17/18] add changeset --- .changeset/cyan-books-melt.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/cyan-books-melt.md diff --git a/.changeset/cyan-books-melt.md b/.changeset/cyan-books-melt.md new file mode 100644 index 00000000000..0e3721fbec9 --- /dev/null +++ b/.changeset/cyan-books-melt.md @@ -0,0 +1,5 @@ +--- +'@firebase/firestore': patch +--- + +Add set() override for merge options in Firestore Lite SDK From 4323e7e11547876fe9eaa142cb3c8a03d03cc4ff Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Fri, 26 Jun 2020 15:50:56 -0700 Subject: [PATCH 18/18] empty changeset --- .changeset/cyan-books-melt.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/.changeset/cyan-books-melt.md b/.changeset/cyan-books-melt.md index 0e3721fbec9..ec380ec43f2 100644 --- a/.changeset/cyan-books-melt.md +++ b/.changeset/cyan-books-melt.md @@ -1,5 +1,3 @@ --- -'@firebase/firestore': patch --- -Add set() override for merge options in Firestore Lite SDK