feat(firestore): start withConverter feature

[Ehesp]

Description

Adds in the withConverter functionality to Firestore.

Related issues

https://invertase.canny.io/admin/board/react-native-firebase/p/support-firestore-dataconverter-withconverter

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;
  • [ ] 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
  • [x] No

Test Plan

Need to test both E2E, unit tests & also add in some TypeScript tests, I'm not 100% sure yet how the types filter down to the returned documents... eg:

const collection = firestore.collection('foo').withConverter<ExpectedResult>({});

const docSnapshot = await collection.get();

// Should return type of "docSnapshot.data()" now be ExpectedResult?

[Salakar]

DocumentSnapshot type would need changing to add in T,

E.g.

interface DocumentSnapshot<T = DocumentSnapshotDataType> {
  // .. 
  data(): T;
}

DocumentSnapshotDataType being whatever it currently returns without the converter changes

[andrefedev]

I am migrating my application to react native and I am using this package, I am concerned that such a necessary function in the Typescript world has not been implemented yet. Any other alternative to handle this?

[mikehardy]

There are no plans @andrefedev - it's a very limited crew (basically, me, very part time) where I have the main task of shepherding community contributions to successful testing and release so that the package moves forward effectively but only so far as the community contributes.

As such I would be more than happy if you picked up this PR and brought it to completion, and I would assist in doing so, but someone from the community with sufficient interest would have to be the prime mover on it, otherwise we infer that it's not important enough and it sits.

The original author - Eliot - is booked on other things as I understand it - so picking it up would be welcome to everyone

[anhnch]

I can't wait for this feature

[mikehardy]

@anhnch PRs gladly accepted - you may use this as a basis for work, just let us know and we'll coordinate for merge/release

[dogansalman]

I can't wait for this feature

+1

[vercel[bot]]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/F7QGAKLsFs4K4jii4SStcQRJZ3rp
✅ Preview: https://react-native-f-git-feat-firestore-with-converter-in-f1c5db.vercel.app

[codecov[bot]]

Codecov Report

Merging #3546 (d9e5548) into master (0d6cbb8) will decrease coverage by 52.28%. The diff coverage is n/a.

:exclamation: Current head d9e5548 differs from pull request most recent head d17eb21. Consider uploading reports for the commit d17eb21 to get more accurate results

@@             Coverage Diff             @@
##           master    #3546       +/-   ##
===========================================
- Coverage   88.94%   36.66%   -52.27%     
===========================================
  Files         109       51       -58     
  Lines        3741     1514     -2227     
  Branches      358      358               
===========================================
- Hits         3327      555     -2772     
- Misses        369      731      +362     
- Partials       45      228      +183     

[JanErikFoss]

What's the status here?

[mikehardy]

All development here is out in the open, the comments you see are the status there is

[jthoward64]

What is needed to get this merged? To get this going is it just tests and updating it with main? Or is there functionality missing? Basically, what comments on here are still relevant?

[mikehardy]

I'm unsure actually - firestore files have not changed too drastically so the merge conflict is hopefully not too bad to resolve. [edit: yeah - addition of queryName parameter in one file, addition of undefined-allowed setting in another file, pretty reasonable to fix]

@Ehesp has been poking around the repo the last couple days, he may have current thoughts on this?

[github-actions[bot]]

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

[superphil0]

Is this still in the making or did this get dropped from the roadmap?

[mikehardy]

It's an exciting feature, but the current roadmap is - "finish firebase-js-sdk v9 adoption", "typescript", "support react-native-web (somehow)". This feature may make it's way inside that list somewhere but that list is where we are focusing at the moment

[ChromeQ]

I would hope that this feature falls under the "typescript" project on the roadmap @mikehardy Converting a project from web to native means it isn't as straightforward as I hoped on the firestore stuff, looking forward to seeing more on this making progress. Is the "solution" for now to cast as Type? And also manually call the conversion functions (Timestamp to Date etc)?

[jthoward64]

I would hope that this feature falls under the "typescript" project on the roadmap @mikehardy Converting a project from web to native means it isn't as straightforward as I hoped on the firestore stuff, looking forward to seeing more on this making progress. Is the "solution" for now to cast as Type? And also manually call the conversion functions (Timestamp to Date etc)?

That or wrap the calls to firestore and use as unknown and then throw it through a type check function

[haeniya]

@ChromeQ How did you go about this? Was it possible to just call the conversion functions manually? I'm thinking about migrating from web to native but this currently hinders me because I'm not really sure how to do it best.

[ChromeQ]

@haeniya I just used a TS cast to the type I know it should be (although it should be safer to do as @jthoward64 suggests to have a runtime check with a type check function)

// Ignore this - this is what you can do _instead_ of the "solution" below, this is the "lazy" way to simply cast
const snapshot = await firestore.collection('days').doc(day).get();
const { pins } = snapshot.data() as { pins: DocumentReference<FromFirestorePin>[] };

Below here 👇️ is a "solution" to work around the missing converters in react-native-firebase. I have defined two types which represent the differences between a front-end ready json object and the actual return from firestore:

export interface ToFirestorePin extends Omit<Pin, 'updatedAt'> {
  updatedAt: FirebaseFirestoreTypes.FieldValue;
}

export interface FromFirestorePin extends Omit<Pin, 'updatedAt'> {
  updatedAt: FirebaseFirestoreTypes.Timestamp;
}

And then I also have a "conversion" function like this:

export const convertFirestorePinToPin = (
  doc:
    | FirebaseFirestoreTypes.QueryDocumentSnapshot<FromFirestorePin>
    | FirebaseFirestoreTypes.DocumentSnapshot<FromFirestorePin>
): Pin => {
  const data = doc.data();

  if (!data) {
    throw new Error(`No data found for firestore document`);
  }

  return {
    ...data,
    id: data.id,
    updatedAt: data.updatedAt.toDate().toISOString(),
  };
};

You can just call this:

const snapshot = await firestore.collection<FromFirestorePin>('pins').doc(pinId).get();
const pin = convertFirestorePinToPin(snapshot);

Hope this helps and I am only converting the Timestamp to a UTC ISO string but there are a few other differences that might trip you up. Good luck

[haeniya]

@ChromeQ many thanks! That should get me started.