feat(firestore): implement withCoverter
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)
- [x]
- My change includes tests;
- [x]
e2etests added or updated inpackages/\*\*/e2e - [x]
jesttests added or updated inpackages/\*\*/__tests__
- [x]
- [x] I have updated TypeScript types that are affected by my change.
- This is a breaking change;
- [x] Yes
- [ ] No
:fire:
@JiriHoffmann is attempting to deploy a commit to the Invertase Team on Vercel.
A member of the Team first needs to authorize it.
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
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
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
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
Updated this PR to include Firestore settings changes for tests from https://github.com/invertase/react-native-firebase/issues/8749
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)
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
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.