react-native-firebase icon indicating copy to clipboard operation
react-native-firebase copied to clipboard

feat(firestore): implement withCoverter

Open JiriHoffmann opened this issue 3 months ago • 10 comments

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 https://github.com/invertase/react-native-firebase/issues/8611 and these 2 PRs https://github.com/invertase/react-native-firebase/pull/8698 https://github.com/invertase/react-native-firebase/pull/8672

Release Summary

Implements withConverter from Firebase JS SDK

Checklist

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

:fire:

JiriHoffmann avatar Nov 07 '25 13:11 JiriHoffmann

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

A member of the Team first needs to authorize it.

vercel[bot] avatar Nov 07 '25 13:11 vercel[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Nov 07 '25 13:11 CLAassistant

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

mikehardy avatar Nov 07 '25 17:11 mikehardy

Looks like there are a couple things going on here in the CI failures -

1- there's an error in types-test with an unused 'doc' element

  • /home/runner/work/react-native-firebase/react-native-firebase/website/scripts/generate-typedoc.js:37:18

2- there's some use of non-modular code in the new modular e2e tests

  • /Users/runner/work/react-native-firebase/react-native-firebase/packages/firestore/e2e/withConverter.e2e.js:274:17

3- Then, seemingly unrelated to this PR, there's a problem with the ios build where some sort of entropy ("things just fail over time for unrelated reasons") has taken over and resulted in

xcodebuild: error: Found no destinations for the scheme 'testing' and action build.

1 and 2 are for this PR, I'll tackle 3 separately and either cherry-pick or rebase it into this PR, coordinating with you as/when I do so

mikehardy avatar Nov 09 '25 13:11 mikehardy

This PR will merge as soon as it goes green, and I'll rebase this PR on top of it, at which point this PR should go green...

  • #8748

mikehardy avatar Nov 10 '25 20:11 mikehardy

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

mikehardy avatar Nov 10 '25 21:11 mikehardy

Updated this PR to include Firestore settings changes for tests from https://github.com/invertase/react-native-firebase/issues/8749

JiriHoffmann avatar Nov 20 '25 21:11 JiriHoffmann

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 avatar Nov 21 '25 14:11 mikehardy

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 avatar Nov 28 '25 22:11 mikehardy

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 avatar Nov 29 '25 15:11 JiriHoffmann