[Feat] Add bundler implementation to Firestore SSR Serialization feature branch #8872

DellaBitta · 2025-03-27T13:41:22Z

Discussion

Merge the initial bundler implementation into the Firestore Result Serialization feature branch.
Implementation includes the ability to invoke toJSON() on QuerySnapshot and DocumentSnapshot instances.

Testing

Local tests to plumb the resulting bundle result into loadBundle on the client. Executed getDocFromCache to ensure that the document could be retrieved.

Also tested a new instance of Firestore by storing the bundle string in a file and executing a different node application with the string. This ensured that the cache didn't already have the original document from the result of getDoc or getDocs server provided results.

Added unit and integration tests.

API Changes

Waiting for the full feature branch to be complete before submitting an API review doc. This change includes the new methods:

QuerySnapshot.toJSON()
DocumentSnapshot.toJSON()

General type errors. Removed QualifiedResourcePath.

…ormat (#8855) Co-authored-by: Mark Duckworth <[email protected]>

Tested the results in loadBundle with a document consisting of all data types, and it succeeded.

name and parent fields aren't properly populated. Looks like we have extraneous "orderBy" fields, too, which might be harmless.

Also includes: - format - lint fixes. - Comments for bundler methods - Addition of DocumentSnapshotBundleData & QuerySnapshotBundleData types.

changeset-bot · 2025-03-27T13:41:26Z

⚠️ No Changeset found

Latest commit: a5b46b7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

google-oss-bot · 2025-03-27T13:50:54Z

Size Report ¹

Affected Products

@firebase/firestore
Type Base (c8cbfff) Merge (4aa6dd5) Diff
browser 382 kB 387 kB +5.67 kB (+1.5%)
main 591 kB 599 kB +7.81 kB (+1.3%)
module 382 kB 387 kB +5.67 kB (+1.5%)
react-native 382 kB 388 kB +5.67 kB (+1.5%)

`bundle`

Type	Base (`c8cbfff`)	Merge (`4aa6dd5`)	Diff
firestore (CSI Auto Indexing Disable and Delete)	272 kB	274 kB	+1.77 kB (+0.6%)
firestore (CSI Auto Indexing Enable)	272 kB	274 kB	+1.77 kB (+0.6%)
firestore (Persistence)	304 kB	305 kB	+1.77 kB (+0.6%)
firestore (Query Cursors)	250 kB	255 kB	+4.98 kB (+2.0%)
firestore (Query)	248 kB	253 kB	+4.98 kB (+2.0%)
firestore (Read data once)	236 kB	244 kB	+8.57 kB (+3.6%)
firestore (Read Write w Persistence)	328 kB	332 kB	+4.25 kB (+1.3%)
firestore (Realtime updates)	238 kB	247 kB	+9.25 kB (+3.9%)
firestore (Transaction)	215 kB	221 kB	+6.42 kB (+3.0%)
firestore (Write data)	214 kB	216 kB	+1.77 kB (+0.8%)

firebase
Type Base (c8cbfff) Merge (4aa6dd5) Diff
firebase-compat.js 794 kB 799 kB +4.86 kB (+0.6%)
firebase-firestore-compat.js 340 kB 345 kB +4.86 kB (+1.4%)
firebase-firestore.js 440 kB 446 kB +5.68 kB (+1.3%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/6YJPs0dpK4.html

google-oss-bot · 2025-03-27T14:02:21Z

Size Analysis Report ¹

This report is too large (770,132 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/tViHxc9uE7.html

These were added to the API before I knew they could have been queried internally.

removed extraneous imports and private/protected changes.

MarkDuckworth

Overall, this is looking good. I have a few comments that we may need to address

MarkDuckworth · 2025-03-27T18:56:16Z

packages/firestore/src/util/bundle_builder_impl.ts

+    if (
+      !originalDocument ||
+      (!readTime && !originalDocument.metadata.readTime) ||
+      (readTime && originalDocument.metadata.readTime! < readTime)


what happens if originalDocument.metadata.readTime is null|undefined?

Maybe this is a direct port from nodejs?

Ok, I cleaned this up quite a bit. PTAL.

MarkDuckworth · 2025-03-27T18:58:27Z

packages/firestore/src/util/bundle_builder_impl.ts

+    if (queryName) {
+      const newDocument = this.documents.get(docBundleData.documentPath)!;
+      newDocument.metadata.queries = originalQueries || [];
+      if (queryName) {


I think this will always be true because it's nested inside line 122

Too right. Removed.

MarkDuckworth · 2025-03-27T18:59:38Z

packages/firestore/src/util/bundle_builder_impl.ts

+   * QuerySnapshot. Note we cannot accept a QuerySnapshot directly due to a circular
+   * dependency error.


nice fix for the circular dep

MarkDuckworth · 2025-03-27T19:14:08Z

packages/firestore/src/util/bundle_builder_impl.ts

+    debugAssert(
+      !docBundleData.documentData.hasLocalMutations,
+      "Can't serialize documents with mutations."
+    );


We will have to consider the impact of this assert on serializing documents with local mutations. This could be a breaking change if someone is already performing JSON.stringify(mutatedDoc). This is a use case that should not matter for SSR, but will matter because we are using toJSON for serialization.

I guess there's also a case where someone writes or modifies a document on the server side, and then runs a query to get that document (with modifications) before the server gets a write ack from the backend. This means the documentsnapshot would be with mutations...

I'm not even sure how SSR should behave in this case.

This is a tricky situation for SSR. We will have to be clear that the server side code must use getDocumentsFromServer to ensure that we don't get a document with local mutation.

Also, I'm beginning to wonder if we should only override toJSON for these snapshot types when running server side. This helps avoid dealing with edge cases of serialization in the client side. Thoughts?

We discussed this offline, and resolved to throw in these situations. I've updated the branch accordingly. PTAL.

MarkDuckworth · 2025-03-27T19:38:43Z

packages/firestore/src/util/bundle_builder_validation_utils.ts

+ * @param value The input to validate.
+ * @param options Options that specify whether the string can be omitted.
+ */
+export function validateString(arg: string | number, value: unknown): void {


it's not clear to me if any of the functions in this file are used or will be used?

Oh odd, I thought I had removed this source file. I must have messed up the git push. Will fix.

MarkDuckworth

LGTM. One suggestion to correct the error message

MarkDuckworth · 2025-03-28T20:06:01Z

packages/firestore/src/api/snapshot.ts

+      throw new FirestoreError(
+        Code.FAILED_PRECONDITION,
+        'DocumentSnapshot.toJSON attempted to serialize a document with pending writes. ' +
+          'Await `waitForPendingWrites()` before invoking `toJSON()`.'


I think it needs to be 'Await waitForPendingWrites()` before getting documents.'

MarkDuckworth · 2025-03-28T20:06:13Z

packages/firestore/src/api/snapshot.ts

+        throw new FirestoreError(
+          Code.FAILED_PRECONDITION,
+          'QuerySnapshot.toJSON attempted to serialize a document with pending writes. ' +
+            'Await `waitForPendingWrites()` before invoking `toJSON()`.'


DellaBitta and others added 12 commits March 14, 2025 16:33

first attempt

aee01c3

Build fixes

c18ddd5

General type errors. Removed QualifiedResourcePath.

Add Firestore instance to constructor

9ebb633

lint fixes

3c2e837

first addQuery impl

71d24b4

reduced tools in validation utils.

5e9ceb8

remove unneeded utils.

5dd5ae4

Ideas to support converting bundle protos to Proto3 JSON compatible f…

3d45407

…ormat (#8855) Co-authored-by: Mark Duckworth <[email protected]>

Format and coalesce utils.

c361963

Bundle DocumentSnaps with toJSON

a1e6a0b

Tested the results in loadBundle with a document consisting of all data types, and it succeeded.

QuerySnapshot toJSON impl, but it has bugs

5e3620d

name and parent fields aren't properly populated. Looks like we have extraneous "orderBy" fields, too, which might be harmless.

Successfully tested bundling a QuerySnapshot

ffff490

Also includes: - format - lint fixes. - Comments for bundler methods - Addition of DocumentSnapshotBundleData & QuerySnapshotBundleData types.

DellaBitta added 3 commits March 27, 2025 10:07

Remove createTime & readTime from DocumentSnapshot

3bcf31e

These were added to the API before I knew they could have been queried internally.

rename documentToDocumentSnapshotBundleData

ccd5205

Cleanup pass

03f3ffb

removed extraneous imports and private/protected changes.

MarkDuckworth reviewed Mar 27, 2025

View reviewed changes

DellaBitta added 7 commits March 28, 2025 08:48

Remove unneeded bundle_builder_validation_utils.ts

06d84e0

clean up addBundleDocument readTime logic.

1b53351

removed extra if(queryName) in addBundleDocument

5f413b6

Update fromTimestamp doc.

e0f3c29

toJSON methods warn of pending writes.

fbfa24f

throw instead of warn.

070a84a

format

8e0b3bb

DellaBitta requested a review from MarkDuckworth March 28, 2025 19:12

metadata contains hasPendingWrites

65c3d8b

MarkDuckworth approved these changes Mar 28, 2025

View reviewed changes

Unit tests.

73eb8a2

DellaBitta added 5 commits March 28, 2025 16:58

Update database.test.ts

6251a28

Add integration test.

2a3a7df

Merge branch 'main' into ddb-migrate-fs-bundler

f009e35

Docs

afd294a

debug logs

06f71c9

DellaBitta changed the title ~~[WIP] Add bundler to Firestore SSR Serialization feature branch~~ [Feat] Add bundler implementation to Firestore SSR Serialization feature branch Apr 1, 2025

DellaBitta added 4 commits April 1, 2025 09:49

Guard test against missing path.

4ec62ae

bundle test

36e4fd5

Minify fix.

bad6f72

lint fixes, console log removal.

a5b46b7

DellaBitta marked this pull request as ready for review April 1, 2025 19:22

DellaBitta requested review from a team as code owners April 1, 2025 19:22

DellaBitta merged commit b85747b into ddb-firestore-result-serialization Apr 1, 2025
39 of 45 checks passed

DellaBitta deleted the ddb-migrate-fs-bundler branch April 1, 2025 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feat] Add bundler implementation to Firestore SSR Serialization feature branch #8872

[Feat] Add bundler implementation to Firestore SSR Serialization feature branch #8872

DellaBitta commented Mar 27, 2025 •

edited

Loading

changeset-bot bot commented Mar 27, 2025 •

edited

Loading

google-oss-bot commented Mar 27, 2025 •

edited

Loading

`@firebase/firestore`

`bundle`

`firebase`

google-oss-bot commented Mar 27, 2025 •

edited

Loading

MarkDuckworth left a comment

MarkDuckworth Mar 27, 2025

DellaBitta Mar 28, 2025

MarkDuckworth Mar 27, 2025

DellaBitta Mar 28, 2025

MarkDuckworth Mar 27, 2025

MarkDuckworth Mar 27, 2025

MarkDuckworth Mar 27, 2025

MarkDuckworth Mar 27, 2025

DellaBitta Mar 28, 2025

MarkDuckworth Mar 27, 2025

DellaBitta Mar 27, 2025 •

edited

Loading

DellaBitta Mar 28, 2025

MarkDuckworth left a comment

MarkDuckworth Mar 28, 2025

MarkDuckworth Mar 28, 2025

		* QuerySnapshot. Note we cannot accept a QuerySnapshot directly due to a circular
		* dependency error.

[Feat] Add bundler implementation to Firestore SSR Serialization feature branch #8872

[Feat] Add bundler implementation to Firestore SSR Serialization feature branch #8872

Conversation

DellaBitta commented Mar 27, 2025 • edited Loading

Discussion

Testing

API Changes

changeset-bot bot commented Mar 27, 2025 • edited Loading

⚠️ No Changeset found

google-oss-bot commented Mar 27, 2025 • edited Loading

Size Report 1

Affected Products

@firebase/firestore

bundle

firebase

Test Logs

google-oss-bot commented Mar 27, 2025 • edited Loading

Size Analysis Report 1

Test Logs

MarkDuckworth left a 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

DellaBitta Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkDuckworth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DellaBitta commented Mar 27, 2025 •

edited

Loading

changeset-bot bot commented Mar 27, 2025 •

edited

Loading

google-oss-bot commented Mar 27, 2025 •

edited

Loading

Size Report ¹

`@firebase/firestore`

`bundle`

`firebase`

google-oss-bot commented Mar 27, 2025 •

edited

Loading

Size Analysis Report ¹

DellaBitta Mar 27, 2025 •

edited

Loading