From 2e1de6a2b7188529c804c1eb0fddbc89b0415d16 Mon Sep 17 00:00:00 2001 From: cxam Date: Tue, 8 Jan 2019 14:49:51 +0000 Subject: [PATCH 1/8] Overload getAll function to support destructuring on first param --- dev/src/index.ts | 2 ++ dev/src/transaction.ts | 2 ++ 2 files changed, 4 insertions(+) diff --git a/dev/src/index.ts b/dev/src/index.ts index 52b37e2c7..a3b6bc4a4 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -703,6 +703,8 @@ export class Firestore { * console.log(`Second document: ${JSON.stringify(docs[1])}`); * }); */ + getAll(...documentRefsOrReadOptions: Array): + Promise; getAll( documentRef: DocumentReference, ...moreDocumentRefsOrReadOptions: Array): diff --git a/dev/src/transaction.ts b/dev/src/transaction.ts index dd2cf7333..c6158cb96 100644 --- a/dev/src/transaction.ts +++ b/dev/src/transaction.ts @@ -157,6 +157,8 @@ export class Transaction { * }); * }); */ + getAll(...documentRefsOrReadOptions: Array): + Promise; getAll( documentRef: DocumentReference, ...moreDocumentRefsOrReadOptions: Array): From 537cf86ed4da0858b03956d8afaf268d2926039a Mon Sep 17 00:00:00 2001 From: cxam Date: Tue, 8 Jan 2019 14:50:13 +0000 Subject: [PATCH 2/8] Update tests for getAll array destructuring --- dev/system-test/firestore.ts | 59 ++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts index cda8b5d04..173412b31 100644 --- a/dev/system-test/firestore.ts +++ b/dev/system-test/firestore.ts @@ -77,6 +77,18 @@ describe('Firestore class', () => { }); }); + it('getAll() supports array destructuring', () => { + const ref1 = randomCol.doc('doc1'); + const ref2 = randomCol.doc('doc2'); + return Promise.all([ref1.set({foo: 'a'}), ref2.set({foo: 'a'})]) + .then(() => { + return firestore.getAll(...[ref1, ref2]); + }) + .then(docs => { + expect(docs.length).to.equal(2); + }); + }); + it('getAll() supports field mask', () => { const ref1 = randomCol.doc('doc1'); return ref1.set({foo: 'a', bar: 'b'}) @@ -87,6 +99,19 @@ describe('Firestore class', () => { expect(docs[0].data()).to.deep.equal({foo: 'a'}); }); }); + + it('getAll() supports array destructuring with field mask', () => { + const ref1 = randomCol.doc('doc1'); + const ref2 = randomCol.doc('doc2'); + return Promise.all([ref1.set({f: 'a', b: 'b'}), ref2.set({f: 'a', b: 'b'})]) + .then(() => { + return firestore.getAll(...[ref1, ref2], {fieldMask: ['f']}); + }) + .then(docs => { + expect(docs[0].data()).to.deep.equal({f: 'a'}); + expect(docs[1].data()).to.deep.equal({f: 'a'}); + }); + }); }); describe('CollectionReference class', () => { @@ -1383,6 +1408,22 @@ describe('Transaction class', () => { }); }); + it('getAll() supports array destructuring', () => { + const ref1 = randomCol.doc('doc1'); + const ref2 = randomCol.doc('doc2'); + return Promise.all([ref1.set({}), ref2.set({})]) + .then(() => { + return firestore.runTransaction(updateFunction => { + return updateFunction.getAll(...[ref1, ref2]).then(docs => { + return Promise.resolve(docs.length); + }); + }); + }) + .then(res => { + expect(res).to.equal(2); + }); + }); + it('getAll() supports field mask', () => { const ref1 = randomCol.doc('doc1'); return ref1.set({foo: 'a', bar: 'b'}).then(() => { @@ -1397,6 +1438,24 @@ describe('Transaction class', () => { }); }); + it('getAll() supports array destructuring with field mask', () => { + const ref1 = randomCol.doc('doc1'); + const ref2 = randomCol.doc('doc2'); + return Promise.all([ref1.set({f: 'a', b: 'b'}), ref2.set({f: 'a', b: 'b'})]) + .then(() => { + return firestore.runTransaction(updateFunction => { + return updateFunction.getAll(...[ref1, ref2], {fieldMask: ['f']}) + .then(docs => { + expect(docs[0].data()).to.deep.equal({f: 'a'}); + expect(docs[1].data()).to.deep.equal({f: 'a'}); + }); + }); + }) + .then(res => { + expect(res).to.equal(2); + }); + }); + it('has get() with query', () => { const ref = randomCol.doc('doc'); const query = randomCol.where('foo', '==', 'bar'); From 0ef67977cd6f8da650ef30be7aa2327fea2064c3 Mon Sep 17 00:00:00 2001 From: cxam Date: Tue, 8 Jan 2019 14:50:56 +0000 Subject: [PATCH 3/8] Include getAll overload in types --- types/firestore.d.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 87f6e6b60..6d3e7dc91 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -146,6 +146,8 @@ declare namespace FirebaseFirestore { * @return A Promise that resolves with an array of resulting document * snapshots. */ + getAll(...documentRefsOrReadOptions: Array): + Promise; getAll( documentRef: DocumentReference, ...moreDocumentRefsOrReadOptions: Array @@ -266,6 +268,8 @@ declare namespace FirebaseFirestore { * @return A Promise that resolves with an array of resulting document * snapshots. */ + getAll(...documentRefsOrReadOptions: Array): + Promise; getAll( documentRef: DocumentReference, ...moreDocumentRefsOrReadOptions: Array From 34531326c43f3487b16e2921b6277902408def9f Mon Sep 17 00:00:00 2001 From: cxam Date: Tue, 8 Jan 2019 19:59:32 +0000 Subject: [PATCH 4/8] Update getAll to use tuple type with rest element --- dev/src/index.ts | 13 +++++-------- dev/src/transaction.ts | 13 +++++-------- types/firestore.d.ts | 16 ++++++---------- 3 files changed, 16 insertions(+), 26 deletions(-) diff --git a/dev/src/index.ts b/dev/src/index.ts index a3b6bc4a4..419e69b2f 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -703,16 +703,13 @@ export class Firestore { * console.log(`Second document: ${JSON.stringify(docs[1])}`); * }); */ - getAll(...documentRefsOrReadOptions: Array): - Promise; - getAll( - documentRef: DocumentReference, - ...moreDocumentRefsOrReadOptions: Array): - Promise { + getAll(...documentRefsOrReadOptions: [ + DocumentReference, ...Array + ]): Promise { this._validator.minNumberOfArguments('Firestore.getAll', arguments, 1); - const {documents, fieldMask} = parseGetAllArguments( - this._validator, [documentRef, ...moreDocumentRefsOrReadOptions]); + const {documents, fieldMask} = + parseGetAllArguments(this._validator, [...documentRefsOrReadOptions]); return this.getAll_(documents, fieldMask, requestTag()); } diff --git a/dev/src/transaction.ts b/dev/src/transaction.ts index c6158cb96..80aef6fec 100644 --- a/dev/src/transaction.ts +++ b/dev/src/transaction.ts @@ -157,20 +157,17 @@ export class Transaction { * }); * }); */ - getAll(...documentRefsOrReadOptions: Array): - Promise; - getAll( - documentRef: DocumentReference, - ...moreDocumentRefsOrReadOptions: Array): - Promise { + getAll(...documentRefsOrReadOptions: [ + DocumentReference, ...Array + ]): Promise { if (!this._writeBatch.isEmpty) { throw new Error(READ_AFTER_WRITE_ERROR_MSG); } this._validator.minNumberOfArguments('Transaction.getAll', arguments, 1); - const {documents, fieldMask} = parseGetAllArguments( - this._validator, [documentRef, ...moreDocumentRefsOrReadOptions]); + const {documents, fieldMask} = + parseGetAllArguments(this._validator, [...documentRefsOrReadOptions]); return this._firestore.getAll_( documents, fieldMask, this._requestTag, this._transactionId); diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 6d3e7dc91..5fd89c16e 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -146,12 +146,10 @@ declare namespace FirebaseFirestore { * @return A Promise that resolves with an array of resulting document * snapshots. */ - getAll(...documentRefsOrReadOptions: Array): - Promise; getAll( - documentRef: DocumentReference, - ...moreDocumentRefsOrReadOptions: Array - ): Promise; + ...documentRefsOrReadOptions: [ + DocumentReference, ...Array + ]): Promise; /** * Fetches the root collections that are associated with this Firestore @@ -268,12 +266,10 @@ declare namespace FirebaseFirestore { * @return A Promise that resolves with an array of resulting document * snapshots. */ - getAll(...documentRefsOrReadOptions: Array): - Promise; getAll( - documentRef: DocumentReference, - ...moreDocumentRefsOrReadOptions: Array - ): Promise; + ...documentRefsOrReadOptions: [ + DocumentReference, ...Array + ]): Promise; /** * Create the document referred to by the provided `DocumentReference`. From c9ea282950416d5bcc144480b625822e6585bb09 Mon Sep 17 00:00:00 2001 From: cxam Date: Tue, 8 Jan 2019 23:44:06 +0000 Subject: [PATCH 5/8] Revert to original getAll function signature --- dev/src/index.ts | 7 +++---- dev/src/transaction.ts | 7 +++---- types/firestore.d.ts | 12 ++++-------- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/dev/src/index.ts b/dev/src/index.ts index 419e69b2f..e01df6c5c 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -703,13 +703,12 @@ export class Firestore { * console.log(`Second document: ${JSON.stringify(docs[1])}`); * }); */ - getAll(...documentRefsOrReadOptions: [ - DocumentReference, ...Array - ]): Promise { + getAll(...documentRefsOrReadOptions: Array): + Promise { this._validator.minNumberOfArguments('Firestore.getAll', arguments, 1); const {documents, fieldMask} = - parseGetAllArguments(this._validator, [...documentRefsOrReadOptions]); + parseGetAllArguments(this._validator, documentRefsOrReadOptions); return this.getAll_(documents, fieldMask, requestTag()); } diff --git a/dev/src/transaction.ts b/dev/src/transaction.ts index 80aef6fec..1f5b6642d 100644 --- a/dev/src/transaction.ts +++ b/dev/src/transaction.ts @@ -157,9 +157,8 @@ export class Transaction { * }); * }); */ - getAll(...documentRefsOrReadOptions: [ - DocumentReference, ...Array - ]): Promise { + getAll(...documentRefsOrReadOptions: Array): + Promise { if (!this._writeBatch.isEmpty) { throw new Error(READ_AFTER_WRITE_ERROR_MSG); } @@ -167,7 +166,7 @@ export class Transaction { this._validator.minNumberOfArguments('Transaction.getAll', arguments, 1); const {documents, fieldMask} = - parseGetAllArguments(this._validator, [...documentRefsOrReadOptions]); + parseGetAllArguments(this._validator, documentRefsOrReadOptions); return this._firestore.getAll_( documents, fieldMask, this._requestTag, this._transactionId); diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 5fd89c16e..59c97a448 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -146,10 +146,8 @@ declare namespace FirebaseFirestore { * @return A Promise that resolves with an array of resulting document * snapshots. */ - getAll( - ...documentRefsOrReadOptions: [ - DocumentReference, ...Array - ]): Promise; + getAll(...documentRefsOrReadOptions: Array): + Promise; /** * Fetches the root collections that are associated with this Firestore @@ -266,10 +264,8 @@ declare namespace FirebaseFirestore { * @return A Promise that resolves with an array of resulting document * snapshots. */ - getAll( - ...documentRefsOrReadOptions: [ - DocumentReference, ...Array - ]): Promise; + getAll(...documentRefsOrReadOptions: Array): + Promise; /** * Create the document referred to by the provided `DocumentReference`. From 7365e405baf7b6554574ff1ef367c333cf9b2baf Mon Sep 17 00:00:00 2001 From: cxam Date: Wed, 9 Jan 2019 07:56:23 +0000 Subject: [PATCH 6/8] Update JSDoc to match getAll function signature --- dev/src/index.ts | 9 ++++++--- dev/src/transaction.ts | 9 ++++++--- types/firestore.d.ts | 22 +++++++++++++++------- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/dev/src/index.ts b/dev/src/index.ts index e01df6c5c..56483f652 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -687,9 +687,12 @@ export class Firestore { /** * Retrieves multiple documents from Firestore. * - * @param {DocumentReference} documentRef A `DocumentReference` to receive. - * @param {Array.} moreDocumentRefsOrReadOptions - * Additional `DocumentReferences` to receive, followed by an optional field + * The first argument is required and must be of type `DocumentReference` + * followed by any additional `DocumentReference` documents. If used, the + * optional `ReadOptions` must be the last argument. + * + * @param {Array.} documentRefsOrReadOptions + * The `DocumentReferences` to receive, followed by an optional field * mask. * @returns {Promise>} A Promise that * contains an array with the resulting document snapshots. diff --git a/dev/src/transaction.ts b/dev/src/transaction.ts index 1f5b6642d..e9d54f7e1 100644 --- a/dev/src/transaction.ts +++ b/dev/src/transaction.ts @@ -137,9 +137,12 @@ export class Transaction { * Retrieves multiple documents from Firestore. Holds a pessimistic lock on * all returned documents. * - * @param {DocumentReference} documentRef A `DocumentReference` to receive. - * @param {Array.} moreDocumentRefsOrReadOptions - * Additional `DocumentReferences` to receive, followed by an optional field + * The first argument is required and must be of type `DocumentReference` + * followed by any additional `DocumentReference` documents. If used, the + * optional `ReadOptions` must be the last argument. + * + * @param {Array.} documentRefsOrReadOptions + * The `DocumentReferences` to receive, followed by an optional field * mask. * @returns {Promise>} A Promise that * contains an array with the resulting document snapshots. diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 59c97a448..a3475027c 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -139,10 +139,14 @@ declare namespace FirebaseFirestore { /** * Retrieves multiple documents from Firestore. - * - * @param documentRef A `DocumentReference` to receive. - * @param moreDocumentRefsOrReadOptions Additional `DocumentReferences` to - * receive, followed by an optional field mask. + * + * The first argument is required and must be of type `DocumentReference` + * followed by any additional `DocumentReference` documents. If used, the + * optional `ReadOptions` must be the last argument. + * + * @param {Array.} documentRefsOrReadOptions + * The `DocumentReferences` to receive, followed by an optional field + * mask. * @return A Promise that resolves with an array of resulting document * snapshots. */ @@ -258,9 +262,13 @@ declare namespace FirebaseFirestore { * Retrieves multiple documents from Firestore. Holds a pessimistic lock on * all returned documents. * - * @param documentRef A `DocumentReference` to receive. - * @param moreDocumentRefsOrReadOptions Additional `DocumentReferences` to - * receive, followed by an optional field mask. + * The first argument is required and must be of type `DocumentReference` + * followed by any additional `DocumentReference` documents. If used, the + * optional `ReadOptions` must be the last argument. + * + * @param {Array.} documentRefsOrReadOptions + * The `DocumentReferences` to receive, followed by an optional field + * mask. * @return A Promise that resolves with an array of resulting document * snapshots. */ From 069b0cf360f7dbacb05d697da6198cfb209c8d1f Mon Sep 17 00:00:00 2001 From: cxam Date: Wed, 9 Jan 2019 07:57:37 +0000 Subject: [PATCH 7/8] Fix Transaction.getAll() test expect conditions --- dev/system-test/firestore.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts index 173412b31..850858957 100644 --- a/dev/system-test/firestore.ts +++ b/dev/system-test/firestore.ts @@ -1443,16 +1443,16 @@ describe('Transaction class', () => { const ref2 = randomCol.doc('doc2'); return Promise.all([ref1.set({f: 'a', b: 'b'}), ref2.set({f: 'a', b: 'b'})]) .then(() => { - return firestore.runTransaction(updateFunction => { - return updateFunction.getAll(...[ref1, ref2], {fieldMask: ['f']}) - .then(docs => { - expect(docs[0].data()).to.deep.equal({f: 'a'}); - expect(docs[1].data()).to.deep.equal({f: 'a'}); - }); - }); - }) - .then(res => { - expect(res).to.equal(2); + return firestore + .runTransaction(updateFunction => { + return updateFunction + .getAll(...[ref1, ref2], {fieldMask: ['f']}) + .then((docs) => docs); + }) + .then(docs => { + expect(docs[0].data()).to.deep.equal({f: 'a'}); + expect(docs[1].data()).to.deep.equal({f: 'a'}); + }); }); }); From bb74a303f62a2960c4a77ce15492222d51a13efa Mon Sep 17 00:00:00 2001 From: cxam Date: Wed, 9 Jan 2019 17:08:51 +0000 Subject: [PATCH 8/8] Fix style --- types/firestore.d.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/types/firestore.d.ts b/types/firestore.d.ts index a3475027c..f79308dd2 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -139,14 +139,14 @@ declare namespace FirebaseFirestore { /** * Retrieves multiple documents from Firestore. - * - * The first argument is required and must be of type `DocumentReference` - * followed by any additional `DocumentReference` documents. If used, the - * optional `ReadOptions` must be the last argument. - * - * @param {Array.} documentRefsOrReadOptions - * The `DocumentReferences` to receive, followed by an optional field - * mask. + * + * The first argument is required and must be of type `DocumentReference` + * followed by any additional `DocumentReference` documents. If used, the + * optional `ReadOptions` must be the last argument. + * + * @param {Array.} documentRefsOrReadOptions + * The `DocumentReferences` to receive, followed by an optional field + * mask. * @return A Promise that resolves with an array of resulting document * snapshots. */