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

feat(firebase): export and fix modular types

Open dhenneke opened this issue 1 year ago • 4 comments

Description

We use the JS SDK for firebase in react-native but want to switch to @react-native-firebase/firestore to enable offline support. To align the codebase in our react and react-native apps we prefer to use the modular API in both projects. However the types were not yet exported properly for TypeScript usage. We also discovered that certain APIs were missing in the modular implementation.

This PR includes the following changes:

  1. Export the types of the modular API & extend the packages/firestore/type-test.ts with the modular API.
  2. Add an implementation for getDoc, getDocFromCache, and getDocFromServer, and use it in all affected e2e tests (previously getDocs was called for DocumentReferences which worked, but is technically incorrect).
  3. Fix the base type of the queries which prohibited the use of query(collection(...), where(...)).

This only exports the types of the firebase package because that's the only one we currently use and can test.

Related issues

Release Summary

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
  • My change includes tests;
    • [x] e2e tests added or updated in packages/\*\*/e2e
    • [ ] 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;
    • [ ] Yes
    • [x] No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

:fire:

dhenneke avatar Apr 18 '24 08:04 dhenneke

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 18, 2024 8:43am

vercel[bot] avatar Apr 18 '24 08:04 vercel[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 18 '24 08:04 CLAassistant

Received internal feedback: this is okay to merge / no types hold, so just need to deprecate old typo-APIs and delegate them to the new one

mikehardy avatar May 03 '24 01:05 mikehardy

@mikehardy thanks for the review. We shipped our last production build with the modular API by temporarily copying the typings to a *.d.ts file into our codebase. So far we didn't see any issues.

Actually, both getDoc and getDocs are needed and are present in the js-sdk. The difference is that getDoc* is called for DocumentReferences (i.e. getDoc(doc(firestore, 'my-collection', 'my-id')) and returns a DocumentSnapshot for a single document, while getDocs* is called for Querys (i.e. getDocs(collection(firestore, 'my-collection'))) and returns a QuerySnapshot with multiple documents.

Since the implementation in the context of this library is identical (they both call .get() on DocumentReference and Query) it worked when you are not using TypeScript. But with TypeScript, you get type errors and it is also a different API compared to @firebase/firestore. So I don't think there is something we could deprecate.

dhenneke avatar May 03 '24 07:05 dhenneke

Sorry for the delay, but thanks so much for the clear explanation @dhenneke - that makes a lot of sense. I have a breaking change release coming up but I will try to get this out first so it's a very easy upgrade before contemplating the breaking change

(the breaking change isn't much on the code level, just requires Xcode 12.5+ and android minSdk 21+ in general or 23+ for auth - but still easier to just get types in first)

Thanks!

mikehardy avatar May 08 '24 17:05 mikehardy

Thanks 🙂

dhenneke avatar May 08 '24 19:05 dhenneke