feat: Release Bundles #1365

wu-hui · 2020-11-16T01:30:03Z

PR to release bundles. Will merge when we decide to release.

codecov · 2020-11-16T01:36:34Z

Codecov Report

Merging #1365 (2f606da) into master (4acc5dd) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1365      +/-   ##
==========================================
- Coverage   98.51%   98.51%   -0.01%     
==========================================
  Files          32       32              
  Lines       19444    19440       -4     
  Branches     1281     1371      +90     
==========================================
- Hits        19156    19152       -4     
  Misses        284      284              
  Partials        4        4

Impacted Files	Coverage Δ
dev/src/bundle.ts	`94.32% <100.00%> (-0.05%)`	⬇️
dev/src/index.ts	`97.56% <100.00%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4acc5dd...2f606da. Read the comment docs.

schmidt-sebastian

I would expect this PR to contain updates to types/firestore.d.ts. Can you double check what needs to be added there?

wu-hui · 2020-11-16T17:00:14Z

I would expect this PR to contain updates to types/firestore.d.ts. Can you double check what needs to be added there?

Added. Thanks for the reminder.

schmidt-sebastian · 2020-11-16T20:03:36Z

types/firestore.d.ts

+     * Creates a new `BundleBuilder` instance to package selected Firestore data into
+     * a bundle.
+     *
+     * @param bundleId. The id of the bundle. When loaded on clients, client SDKs use this id


In the text part: s/id/ID here and everywhere (including the main sources)

Also drop the period after "bundleId".

schmidt-sebastian · 2020-11-16T20:04:07Z

types/firestore.d.ts

+     * @returns {BundleBuilder} This instance.
+     */
+    add(documentSnapshot: DocumentSnapshot): BundleBuilder;
+    /**


Nit: Missing empty line.

schmidt-sebastian · 2020-11-16T20:05:06Z

types/firestore.d.ts

+     * @param {DocumentSnapshot=} documentSnapshot A document snapshot to add.
+     * @returns {BundleBuilder} This instance.


This is TypeScript. You should drop the JSDoc type markers.

schmidt-sebastian · 2020-11-16T20:05:18Z

types/firestore.d.ts

+     * @param {string=} queryName The name of the query to add.
+     * @param {QuerySnapshot=} querySnapshot A query snapshot to add to the bundle.
+     * @returns {BundleBuilder} This instance.


Drop types.

schmidt-sebastian · 2020-11-16T20:06:32Z

types/firestore.d.ts

+     * @param {DocumentSnapshot=} documentSnapshot A document snapshot to add.
+     * @returns {BundleBuilder} This instance.
+     */
+    add(documentSnapshot: DocumentSnapshot): BundleBuilder;


Should DocumentSnapshot (and QuerySnapshot) below be generic (for converter support)? Does the BundleBuilder respect the converters as it generates the Proto?

All it does is call toDocumentProto() on the snapshot, I don't think it is necessary to make it generic?

There is actually no such thing as a non-generic DocumentSnapshot in our API. The DocumentSnapshot type defaults to DocumentSnapshot<DocumentData>, which means that a custom type such as DocumentSnapshot<Foo> cannot be passed. This should be:

add<T>(documentSnapshot: DocumentSnapshot<T>): BundleBuilder;

Same for QuerySnapshot.

Done, thanks.

schmidt-sebastian · 2020-11-17T16:16:35Z

dev/src/bundle.ts

@@ -42,7 +40,7 @@ export class BundleBuilder {
  // The latest read time among all bundled documents and queries.
  private latestReadTime = new Timestamp(0, 0);

-  constructor(private bundleId: string) {}
+  constructor(public readonly bundleId: string) {}


The TS style guide recommends using only "readonly" instead of "public readonly" as they are equivalent.

schmidt-sebastian · 2020-11-17T16:19:23Z

types/firestore.d.ts

+     * a bundle.
+     *
+     * @param bundleId The ID of the bundle. When loaded on clients, client SDKs use this ID
+     * and the timestamp associated with the built bundle to tell if it has been loaded already.


Suggested change

* and the timestamp associated with the built bundle to tell if it has been loaded already.

* and the timestamp associated with the bundle to tell if it has been loaded already.

or:

* and bundle's timestamp associated to tell if the bundle has been loaded already.

schmidt-sebastian · 2020-11-17T16:20:12Z

types/firestore.d.ts

+
+    /**
+     * Adds a Firestore document snapshot or query snapshot to the bundle.
+     * Both the documents data and the query read time will be included in the bundle.


This comment should only apply to DocumentSnapshots.

schmidt-sebastian · 2020-11-17T16:20:26Z

types/firestore.d.ts

+     * Adds a Firestore document snapshot or query snapshot to the bundle.
+     * Both the documents data and the query read time will be included in the bundle.
+     *
+     * @param documentSnapshot A document snapshot to add.


Suggested change

* @param documentSnapshot A document snapshot to add.

* @param documentSnapshot A `DocumentSnapshot` to add.

schmidt-sebastian · 2020-11-17T16:23:11Z

types/firestore.d.ts

+     * @param {DocumentSnapshot=} documentSnapshot A document snapshot to add.
+     * @returns {BundleBuilder} This instance.
+     */
+    add(documentSnapshot: DocumentSnapshot): BundleBuilder;


There is actually no such thing as a non-generic DocumentSnapshot in our API. The DocumentSnapshot type defaults to DocumentSnapshot<DocumentData>, which means that a custom type such as DocumentSnapshot<Foo> cannot be passed. This should be:

add<T>(documentSnapshot: DocumentSnapshot<T>): BundleBuilder;

Same for QuerySnapshot.

schmidt-sebastian · 2020-11-17T16:23:27Z

types/firestore.d.ts

+     * Adds a Firestore document snapshot or query snapshot to the bundle.
+     * Both the documents data and the query read time will be included in the bundle.


Please update to only refer to QuerySnapshots.

schmidt-sebastian · 2020-11-17T16:24:13Z

types/firestore.d.ts

+    add(queryName: string, querySnapshot: QuerySnapshot): BundleBuilder;
+
+    /**
+     * Builds the bundle into a `Buffer` instance.


Maybe:

Suggested change

* Builds the bundle into a `Buffer` instance.

* Builds the bundle and returns the result as a`Buffer` instance.

schmidt-sebastian · 2020-11-17T22:02:28Z

types/firestore.d.ts


    /**
-     * Adds a Firestore document snapshot or query snapshot to the bundle.
-     * Both the documents data and the query read time will be included in the bundle.
+     * Adds a Firestore query snapshot to the bundle. Both the query data and the query read time


We are also adding the resulting documents, no?

s/query snapshot/QuerySnapshot

schmidt-sebastian · 2020-11-17T22:02:45Z

types/firestore.d.ts

@@ -1924,26 +1924,26 @@ declare namespace FirebaseFirestore {
    readonly bundleId: string;

    /**
-     * Adds a Firestore document snapshot or query snapshot to the bundle.
-     * Both the documents data and the query read time will be included in the bundle.
+     * Adds a Firestore document snapshot to the bundle. Both the documents data and the document


s/document snapshot/DocumentSnapshot

feat: Release Bundles

d626af6

wu-hui requested review from a team as code owners November 16, 2020 01:30

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 16, 2020

product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Nov 16, 2020

wu-hui requested a review from schmidt-sebastian November 16, 2020 01:49

wu-hui assigned schmidt-sebastian Nov 16, 2020

schmidt-sebastian suggested changes Nov 16, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Nov 16, 2020

Add type definitions.

5daa503

wu-hui removed their assignment Nov 16, 2020

wu-hui assigned schmidt-sebastian Nov 16, 2020

schmidt-sebastian reviewed Nov 16, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Nov 16, 2020

Address comments

56cfbec

wu-hui assigned schmidt-sebastian and unassigned wu-hui Nov 17, 2020

Allow user to read bundle ID back.

989cd03

schmidt-sebastian reviewed Nov 17, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui Nov 17, 2020

Use generic snapshots instead.

a55c340

schmidt-sebastian approved these changes Nov 17, 2020

View reviewed changes

schmidt-sebastian removed their assignment Nov 17, 2020

wu-hui and others added 2 commits November 18, 2020 10:34

More fixes to ref docs

966fa83

Merge branch 'master' into wuandy/ReleaseBundle

2f606da

wu-hui merged commit bae82dd into master Dec 2, 2020

crwilcox mentioned this pull request Dec 15, 2020

Upgrading from 4.7.2 to 4.8.0 causes high memory consumption #1375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Release Bundles #1365

feat: Release Bundles #1365

wu-hui commented Nov 16, 2020 •

edited

Loading

codecov bot commented Nov 16, 2020 •

edited

Loading

schmidt-sebastian left a comment

wu-hui commented Nov 16, 2020

schmidt-sebastian Nov 16, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 16, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 16, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 16, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 16, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 17, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 17, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 17, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 17, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 17, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 17, 2020

schmidt-sebastian Nov 17, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 17, 2020

wu-hui Nov 17, 2020

schmidt-sebastian Nov 17, 2020

wu-hui Nov 18, 2020

schmidt-sebastian Nov 17, 2020

wu-hui Nov 18, 2020

		* @param {DocumentSnapshot=} documentSnapshot A document snapshot to add.
		* @returns {BundleBuilder} This instance.

	* and the timestamp associated with the built bundle to tell if it has been loaded already.
	* and the timestamp associated with the bundle to tell if it has been loaded already.

	* @param documentSnapshot A document snapshot to add.
	* @param documentSnapshot A `DocumentSnapshot` to add.

		* Adds a Firestore document snapshot or query snapshot to the bundle.
		* Both the documents data and the query read time will be included in the bundle.

	* Builds the bundle into a `Buffer` instance.
	* Builds the bundle and returns the result as a`Buffer` instance.

feat: Release Bundles #1365

feat: Release Bundles #1365

Conversation

wu-hui commented Nov 16, 2020 • edited Loading

codecov bot commented Nov 16, 2020 • edited Loading

Codecov Report

schmidt-sebastian left a comment

Choose a reason for hiding this comment

wu-hui commented Nov 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-hui commented Nov 16, 2020 •

edited

Loading

codecov bot commented Nov 16, 2020 •

edited

Loading