Skip to content

fix(firestore): let calls to collection on a DocumentReference take a type argument #4686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

sanpoChew
Copy link

@sanpoChew sanpoChew commented Dec 17, 2020

Description

I think this may have just been an oversight on this PR #3810

Was quite annoying having to cast types when working with subcollections so I thought I would open a PR.

Related issues

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • 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
    • No

Test Plan


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

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2020

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Dec 17, 2020

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/mpp1pi4w9
✅ Preview: https://react-native-firebase-git-master.invertase.vercel.app

@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #4686 (406ecfd) into master (c3b4cb0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4686   +/-   ##
=======================================
  Coverage   24.06%   24.06%           
=======================================
  Files         109      109           
  Lines        3712     3712           
  Branches      347      347           
=======================================
  Hits          893      893           
  Misses       2611     2611           
  Partials      208      208           

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there! Thanks for sending this in. It seems like it could work

Unfortunately, we try to maintain fidelity with the firebase-js-sdk API, and this goes a bit past it. Our current API is missing the DocumentData type on the returned CollectionReference, but simply harmonizing the API is the acceptable change here

https://github.com/firebase/firebase-js-sdk/blob/1a8cb40ac47d63105e7013a0d5ed97e12816bced/packages/firestore-types/index.d.ts#L68

@sanpoChew
Copy link
Author

not sure how useful that DocumentData addition would really be though, I suppose what I really need is #3546 if parity is what the aim is

@mikehardy
Copy link
Collaborator

@sanpoChew I think you are right, that PR even seemed like it might work, just needs comments addressed and some cleanup. As mentioned on that PR I'd happily work with you to merge it, or if you wanted to just take this one to firebase-js-sdk parity I'll also collaborate

@sanpoChew
Copy link
Author

@mikehardy thanks for the help, I think I'll close this one for now, might have a look at the withConverter PR in the new year!

@sanpoChew sanpoChew closed this Dec 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants