Skip to content
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: serverTimestampBehavior #5514

Closed
wants to merge 0 commits into from
Closed

Conversation

JanErikFoss
Copy link
Contributor

@JanErikFoss JanErikFoss commented Jul 13, 2021

Description

Implements serverTimestampBehavior as a firestore setting.
It's currently impossible to implement SnapshotOptions as they work in the javascript sdk, this is the next best option in my opinion.

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:

@vercel
Copy link

vercel bot commented Jul 13, 2021

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

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/12bbMBbHWfSbWhheRpsWRQvshyoL
✅ Preview: https://react-native-firebase-git-fork-janerikfoss-master-invertase.vercel.app

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/CxKaWMxp3yoeM9VPFTUscvxWFWpW
✅ Preview: Failed

[Deployment for d434196 failed]

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2021

CLA assistant check
All committers have signed the CLA.

@JanErikFoss JanErikFoss changed the title Implement serverTimestampBehavior feat: serverTimestampBehavior Jul 13, 2021
@JanErikFoss JanErikFoss marked this pull request as ready for review July 13, 2021 08:07
@JanErikFoss
Copy link
Contributor Author

@mikehardy As far as I can tell this is done.

mikehardy
mikehardy previously approved these changes Jul 30, 2021
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.

Not just "done" but with what looks like fantastic test support. This looks really good. Running it through CI now

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Needs Review Pending feedback or review from a maintainer. Workflow: Waiting for User Response Blocked waiting for user response. labels Jul 30, 2021
@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

Merging #5514 (915c062) into master (5039759) will increase coverage by 0.06%.
The diff coverage is 64.75%.

❗ Current head 915c062 differs from pull request most recent head d434196. Consider uploading reports for the commit d434196 to get more accurate results

@@             Coverage Diff              @@
##             master    #5514      +/-   ##
============================================
+ Coverage     67.48%   67.54%   +0.06%     
- Complexity      980      983       +3     
============================================
  Files           189      199      +10     
  Lines          9523     9659     +136     
  Branches       1487     1510      +23     
============================================
+ Hits           6426     6523      +97     
- Misses         2686     2722      +36     
- Partials        411      414       +3     

@JanErikFoss
Copy link
Contributor Author

JanErikFoss commented Jul 30, 2021

Everything green except Vercel for react-native-firebase-next, but I don't have access to the details for that one.

@mikehardy
Copy link
Collaborator

This still LGTM but in trying to fix the vercel deployment issue (which was a commit in the 20 commit delta between when you branched for your 3 and current) I blew up your commit state, sincere apologies. I've got the commits though and pushed them here: #5556

@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Jul 30, 2021
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