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
feat: add support for Typescript Custom Mapping #828
Conversation
64c11b0
to
4eacbce
Compare
Codecov Report
@@ Coverage Diff @@
## master #828 +/- ##
=========================================
Coverage ? 96.58%
=========================================
Files ? 25
Lines ? 15418
Branches ? 1137
=========================================
Hits ? 14891
Misses ? 519
Partials ? 8 Continue to review full report at Codecov.
|
3aa76b2
to
34d7968
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I am super stoked that this coming together.
dev/src/document.ts
Outdated
ref: DocumentReference, | ||
fieldsProto?: ApiMapValue, | ||
ref: DocumentReference<T>, | ||
readonly fieldsProto?: ApiMapValue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am undecided whether this should be fieldsProto
or _ fieldsProto
. This is a public field for internal consumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed to _fieldsProto
so that external developers know that it's not intended for external consumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the edits and trying out the queryOptions + stream stuff on your own!
dev/src/document.ts
Outdated
ref: DocumentReference, | ||
fieldsProto?: ApiMapValue, | ||
ref: DocumentReference<T>, | ||
readonly fieldsProto?: ApiMapValue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed to _fieldsProto
so that external developers know that it's not intended for external consumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final comments, but looks good. We should probably change the title to something less generic... 'generics' are the tool we used to implement withConverter
, but it does not describe the feature very well.
Can you also make the description of the PR a bit more expressive so that users can use it as a guide for how this feature works? |
Fixes #856
Added
withConverter()
methods onCollectionReference
,DocumentReference
, andQuery
that allow developers to use Firestore classes with generics.Example usage: