-
Notifications
You must be signed in to change notification settings - Fork 59
Added support for Timestamp and DocumentReference #28
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
Added support for Timestamp and DocumentReference #28
Conversation
What was the test case you used to manually test this? Can you provide some sample code? |
Also, thank you for making this PR! |
I've simplified and obfuscated our code to produce the example however the method remains the same:
This PR changes are being used internally which is working a treat :) |
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.
Sorry for the delay! I was at Firebase Summit. I made some suggestions to the code.
Signed-off-by: Shaun Smekel <[email protected]>
Thanks for the feedback @laurenzlong. Those changes have been made and are passing our internal tests. |
Looks like travis failed for unrelated reasons. |
Looks good! Seems like Travis is failing because a dependency is failing for Node 11. I'll resolve that separately, before merging this PR. Thanks! |
Hi @laurenzlong any update on this? |
Sorry for the delay! |
This is now a part of v0.1.5 |
Closes #24
This PR is to add serialisation support for
firestore.Timestamp
andDocumentReference
for use inmakeDocumentSnapshot
.This fills in the gaps of functionality for the following:
DocumentReference
as part of theDocumentSnapshot
firestore.Timestamp
can now be used instead of justDate
. This is very useful when defining interfaces and models in TS where setting a timestamp had to useDate
, but when getting the timestamp from the database would befirestore.Timestamp
;Tests are passing however I haven't added any new tests; There's currently no coverage for this part of the package.