feat(firestore): implement withCoverter #8744

JiriHoffmann · 2025-11-07T13:23:11Z

Description

Hello! I'm sorry for introducing such a large PR, but I hope it's gonna be worth it.

This PR adds the withCoverter functionality to both the namespace and modular APIs and updates types accordingly.

Both type and e2e tests are a 1:1 copy of the Firebase JS SDK to ensure feature parity.

Related issues

Closes #8611 and these 2 PRs #8698 #8672

Release Summary

Implements withConverter from Firebase JS SDK

Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
- Yes
My change supports the following platforms;
- Android
- iOS
- Other (macOS, web)
My change includes tests;
- e2e tests added or updated in packages/\*\*/e2e
- jest tests added or updated in packages/\*\*/__tests__
I have updated TypeScript types that are affected by my change.
This is a breaking change;
- Yes
- No

🔥

vercel · 2025-11-07T13:23:16Z

@JiriHoffmann is attempting to deploy a commit to the Invertase Team on Vercel.

A member of the Team first needs to authorize it.

CLAassistant · 2025-11-07T13:23:18Z

All committers have signed the CLA.

mikehardy · 2025-11-07T17:57:31Z

Hello! I'm sorry for introducing such a large PR, but I hope it's gonna be worth it.

That is not something I'm used to people apologizing for, this certainly looks worth it. Thanks! Going in the review queue - additionally - if my patch-package generation workflow is working correctly you should be able to start depending on the work relatively easily already via usage of the patch, which should apply cleanly to the current released version of firestore package on npmjs.com

packages/firestore/e2e/withConverter.e2e.js

mikehardy · 2025-11-10T21:30:00Z

I've fixed the CI issues that were happening with iOS and rebased this PR on to them then force-pushed this PR back out with that new commit history built on the iOS e2e fix

I resolved the conversation on the ignoreUndefinedProperties settings and peeled it off to a separate issue so we can move on with out it

This is now just on me to do a real code review on it, then if it's good - merge and release. Will be a huge feature, there will be much rejoicing

JiriHoffmann · 2025-11-20T21:49:26Z

Updated this PR to include Firestore settings changes for tests from #8749

mikehardy · 2025-11-21T14:59:43Z

The other platform (which is a wrapper for firebase-js-sdk) test failure is interesting - a perfect example of "no good deed unpunished" as you tried to use the suggested initializeFirestore method, but firebase-js-sdk does enforce that this is called only once.

For the e2e case where isOther is true (I think that's how to phrase it...) I guess you just have to directly toggle the setting as it was before. Which is frustrating since you just stopped doing that!

           requires the correct converter for Partial usage:
     FirebaseError: Firestore can only be initialized once per app.
      at construct ([native code]:null:null)
      at _construct (/Users/runner/work/react-native-firebase/react-native-firebase/packages/app/node_modules/@babel/runtime/helpers/construct.js:4:64)
      at Wrapper (/Users/runner/work/react-native-firebase/react-native-firebase/packages/app/node_modules/@babel/runtime/helpers/wrapNativeSuper.js:15:13)
      at construct ([native code]:null:null)
      at _callSuper (http://localhost:8081/index.bundle?platform=macos&dev=true&lazy=true&minify=false&inlineSourceMap=false&modulesOnly=false&runModule=true&app=org.reactjs.native.io-invertase-testing:125161:169)
      at FirebaseError#constructor (/Users/runner/work/react-native-firebase/react-native-firebase/packages/app/node_modules/@firebase/util/dist/index.esm.js:1181:8)
      at construct ([native code]:null:null)
      at _callSuper (http://localhost:8081/index.bundle?platform=macos&dev=true&lazy=true&minify=false&inlineSourceMap=false&modulesOnly=false&runModule=true&app=org.reactjs.native.io-invertase-testing:368717:169)
      at FirestoreError#constructor (/Users/runner/work/react-native-firebase/react-native-firebase/packages/app/node_modules/@firebase/firestore/dist/lite/index.rn.esm.js:239:8)
      at initializeFirestore (/Users/runner/work/react-native-firebase/react-native-firebase/packages/app/node_modules/@firebase/firestore/dist/lite/index.rn.esm.js:4509:52)
      at guard$argument_0 (/Users/runner/work/react-native-firebase/react-native-firebase/packages/firestore/lib/web/RNFBFirestoreModule.js:169:42)
      at guard (/Users/runner/work/react-native-firebase/react-native-firebase/packages/app/lib/internal/web/utils.js:5:11)
      at default.settings (/Users/runner/work/react-native-firebase/react-native-firebase/packages/firestore/lib/web/RNFBFirestoreModule.js:168:16)
      at <anonymous> (/Users/runner/work/react-native-firebase/react-native-firebase/packages/app/lib/internal/registry/nativeModule.js:43:34)
      at FirebaseFirestoreModule#settings (/Users/runner/work/react-native-firebase/react-native-firebase/packages/firestore/lib/index.js:391:31)
      at <anonymous> (/Users/runner/work/react-native-firebase/react-native-firebase/packages/app/lib/common/index.js:729:37)
      at initializeFirestore (/Users/runner/work/react-native-firebase/react-native-firebase/packages/firestore/lib/modular/index.js:217:31)
      at asyncGeneratorStep (/Users/runner/work/react-native-firebase/react-native-firebase/tests/node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:16)
      at _next (/Users/runner/work/react-native-firebase/react-native-firebase/tests/node_modules/@babel/runtime/helpers/asyncToGenerator.js:17:26)
      at Promise$argument_0 (/Users/runner/work/react-native-firebase/react-native-firebase/tests/node_modules/@babel/runtime/helpers/asyncToGenerator.js:22:11)
      at tryCallTwo (/Users/runner/work/react-native-firebase/react-native-firebase/tests/node_modules/promise/setimmediate/core.js:45:6)
      at doResolve (/Users/runner/work/react-native-firebase/react-native-firebase/tests/node_modules/promise/setimmediate/core.js:200:22)
      at Promise (/Users/runner/work/react-native-firebase/react-native-firebase/tests/node_modules/promise/setimmediate/core.js:66:11)
      at <anonymous> (/Users/runner/work/react-native-firebase/react-native-firebase/tests/node_modules/@babel/runtime/helpers/asyncToGenerator.js:14:22)
      at initializeFirestore (/Users/runner/work/react-native-firebase/react-native-firebase/packages/firestore/lib/modular/index.js:214:41)
      at withModifiedUndefinedPropertiesTestDb (/Users/runner/work/react-native-firebase/react-native-firebase/packages/firestore/e2e/withConverter.e2e.js:91:38)

mikehardy · 2025-11-28T22:25:39Z

I just squashed the commits here down to a single commit since the rest were just follow-ons after the first, and rebased this to current main which should have CI deflaked plus have updated SDKs and such

This is in final review now and I'm hoping to merge it in a sec - thanks for your patience

mikehardy

The quality here is amazing in my opinion. It is huge but I looked through it all...it seems not just like it matches firebase-js-sdk types exactly but of course implements the advertised feature and is backed by thorough type unit and e2e testing. Easy +1

My only question is: is this a breaking change or not? I don't think it is, but I'm curious for your thoughts. Related: if in your estimation it is a breaking change currently, is there any way we could put deprecated shims in the types so it wasn't breaking now, and then we could remove them in a future release? Cheers

mikehardy · 2025-11-29T01:40:48Z

packages/firestore/type-test.ts

 ).then();
 onSnapshot(doc(firebase.firestore(), 'foo', 'foo'), () => {});
 onSnapshot(doc(firebase.firestore(), 'foo', 'foo'), {
-  next: (snapshot: FirebaseFirestoreTypes.DocumentSnapshot) => {


I'm a little concerned about the typing items being breaking changes - what are your thoughts on that? Will this be drop in for people when this feature ships, or will some typescript code need updating.

We can issue breaking changes, that isn't a problem. The problem is accidentally issuing a non-major semantic release version with breaking changes - we cannot do that.

That is a good catch, this would be a breaking change. The JS SDK modular and namespace converter types differ slightly, so my thought process was to follow the same type structure. Anything exported through the FirebaseFirestoreTypes namespace belongs to v8, and v9 modular API is exported at the top level just like the JS SDK. In my opinion it would better to keep it separate and as a breaking change instead of trying to mix them together.

The release notes should probably include a comment about this change and I can add documentation as well. With that said, the types now become slightly awkward when using the modular API since DocumentSnapshot, Query, etc.. have top level exports, while, for example, WhereFilterOp and Module still have to be imported through the FirebaseFirestoreTypes namespace.

If the types are awkward but nevertheless follow firebase-js-sdk then no problem - awkward still but we have "taste" reference that is "just follow firebase-js-sdk". If they are awkward but divergent - and especially if we are doing a breaking change for types, then this would be a good time to true them up.

I'll reiterate that I'm not afraid of a breaking change (we're on semver major number 23 after all) I just try to be as empathetic as possible and include clear and succinct "this change, impact is that, likely adaptation is ..." so API users can take advantage of what we already know instead of spending limited daily thinking time having to derive it themselves

So the goal then - if this is a breaking change - is just to do it really well, make good docs, then send it...

Alright, I've added a new docs section for the breaking change. Let me know if it's good.

mikehardy · 2025-11-29T01:42:10Z

packages/firestore/e2e/withConverter.e2e.js

+  if (Platform.other) {
+    db._settings.ignoreUndefinedProperties = value;
+  } else {
+    db.settings({ ignoreUndefinedProperties: value });
+  }


Still can't believe that was necessary - such a trivial part of this whole work and ended up being a pain. Sorry

JiriHoffmann · 2025-11-29T15:57:33Z

Also noticed that packages/firestore/lib/modular/snapshot.d.ts is missing a DocumentData import. Will fix that together with any necessary changes based on the types conversation.

JiriHoffmann · 2025-12-02T12:08:30Z

packages/firestore/lib/modular/index.d.ts

-import CollectionReference = FirebaseFirestoreTypes.CollectionReference;
-import DocumentReference = FirebaseFirestoreTypes.DocumentReference;
-import DocumentData = FirebaseFirestoreTypes.DocumentData;
-import Query = FirebaseFirestoreTypes.Query;
-import FieldValue = FirebaseFirestoreTypes.FieldValue;
-import FieldPath = FirebaseFirestoreTypes.FieldPath;
-import PersistentCacheIndexManager = FirebaseFirestoreTypes.PersistentCacheIndexManager;
-import AggregateQuerySnapshot = FirebaseFirestoreTypes.AggregateQuerySnapshot;
+
+export type PersistentCacheIndexManager = FirebaseFirestoreTypes.PersistentCacheIndexManager;
+export type AggregateQuerySnapshot = FirebaseFirestoreTypes.AggregateQuerySnapshot;
+export type SetOptions = FirebaseFirestoreTypes.SetOptions;
+export type SnapshotListenOptions = FirebaseFirestoreTypes.SnapshotListenOptions;
+export type WhereFilterOp = FirebaseFirestoreTypes.WhereFilterOp;
+export type QueryCompositeFilterConstraint = FirebaseFirestoreTypes.QueryCompositeFilterConstraint;

 /** Primitive types. */
 export type Primitive = string | number | boolean | undefined | null;

+/**
+ * A `DocumentData` object represents the data in a document.
+ * - Same as {@link FirebaseFirestoreTypes.DocumentData}
+ */
+export interface DocumentData {
+  [key: string]: any;
+}
+


I've also changed some of these to be exported individually, in addition to FirebaseFirestoreTypes - in line with JS SDK.

DocumentData redefinition here is to make modular types more readable:

vs

mikehardy added plugin: firestore Firebase Cloud Firestore Workflow: Needs Review Pending feedback or review from a maintainer. type: enhancement Implements a new Feature labels Nov 7, 2025

This comment was marked as resolved.

Sign in to view

JiriHoffmann commented Nov 10, 2025

View reviewed changes

packages/firestore/e2e/withConverter.e2e.js Outdated Show resolved Hide resolved

mikehardy mentioned this pull request Nov 10, 2025

test(ios): pin xcode to older version / fix perf issue in simulators #8748

Merged

9 tasks

This comment was marked as resolved.

Sign in to view

mikehardy force-pushed the feat/firestore-converter branch from ff083c5 to 04d563a Compare November 10, 2025 21:27

mikehardy mentioned this pull request Nov 10, 2025

Alter e2e tests to use and test initializeFirestore calls to change settings #8749

Open

mikehardy mentioned this pull request Nov 19, 2025

feat(firestore): Add data converter #8672

Closed

9 tasks

github-actions bot added the Needs Attention label Nov 20, 2025

feat(firestore): implement withConverter

66103ff

mikehardy force-pushed the feat/firestore-converter branch from 5735836 to 66103ff Compare November 28, 2025 22:24

mikehardy reviewed Nov 29, 2025

View reviewed changes

mikehardy added Workflow: Waiting for User Response Blocked waiting for user response. and removed Workflow: Needs Review Pending feedback or review from a maintainer. Needs Attention labels Nov 29, 2025

github-actions bot added the Needs Attention label Nov 29, 2025

JiriHoffmann added 4 commits December 2, 2025 03:40

refactor(firestore): modular type imports & exports

16af71a

docs(firestore): add migration steps for v24

ad58522

chore(firestore): update test ts-expect-error comments

8565855

chore(docs): migrating-to-v24 spellcheck adjustment

67a3060

JiriHoffmann commented Dec 2, 2025

View reviewed changes

feat(firestore): implement withCoverter #8744

Are you sure you want to change the base?

feat(firestore): implement withCoverter #8744

Uh oh!

Conversation

JiriHoffmann commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Release Summary

Checklist

Uh oh!

vercel bot commented Nov 7, 2025

Uh oh!

CLAassistant commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikehardy commented Nov 7, 2025

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

mikehardy commented Nov 10, 2025

Uh oh!

JiriHoffmann commented Nov 20, 2025

Uh oh!

mikehardy commented Nov 21, 2025

Uh oh!

mikehardy commented Nov 28, 2025

Uh oh!

mikehardy left a comment

Choose a reason for hiding this comment

Uh oh!

mikehardy Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

JiriHoffmann Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

mikehardy Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

JiriHoffmann Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

mikehardy Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

JiriHoffmann commented Nov 29, 2025

Uh oh!

JiriHoffmann Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JiriHoffmann commented Nov 7, 2025 •

edited

Loading

CLAassistant commented Nov 7, 2025 •

edited

Loading

JiriHoffmann Dec 2, 2025 •

edited

Loading