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

Added fix to prevent error for Firestore v5.5.0 #32

Merged
merged 5 commits into from
Nov 12, 2018
Merged

Added fix to prevent error for Firestore v5.5.0 #32

merged 5 commits into from
Nov 12, 2018

Conversation

lucacasonato
Copy link
Contributor

This message shows up now... added the fix as described.

@firebase/firestore: Firestore (5.5.0): 
The behavior for Date objects stored in Firestore is going to change
AND YOUR APP MAY BREAK.
To hide this warning and ensure your app does not break, you need to add the
following code to your app before calling any other Cloud Firestore methods:

  const firestore = firebase.firestore();
  const settings = {/* your settings... */ timestampsInSnapshots: true};
  firestore.settings(settings);

With this change, timestamps stored in Cloud Firestore will be read back as
Firebase Timestamp objects instead of as system Date objects. So you will also
need to update code expecting a Date to instead expect a Timestamp. For example:

  // Old:
  const date = snapshot.get('created_at');
  // New:
  const timestamp = snapshot.get('created_at');
  const date = timestamp.toDate();

Please audit all existing usages of Date when you enable the new behavior. In a
future release, the behavior will change to the new behavior, so if you do not
follow these steps, YOUR APP MAY BREAK.

This message shows up now... added the fix as described.

```
@firebase/firestore: Firestore (5.5.0): 
The behavior for Date objects stored in Firestore is going to change
AND YOUR APP MAY BREAK.
To hide this warning and ensure your app does not break, you need to add the
following code to your app before calling any other Cloud Firestore methods:

  const firestore = firebase.firestore();
  const settings = {/* your settings... */ timestampsInSnapshots: true};
  firestore.settings(settings);

With this change, timestamps stored in Cloud Firestore will be read back as
Firebase Timestamp objects instead of as system Date objects. So you will also
need to update code expecting a Date to instead expect a Timestamp. For example:

  // Old:
  const date = snapshot.get('created_at');
  // New:
  const timestamp = snapshot.get('created_at');
  const date = timestamp.toDate();

Please audit all existing usages of Date when you enable the new behavior. In a
future release, the behavior will change to the new behavior, so if you do not
follow these steps, YOUR APP MAY BREAK.
```
@codecov
Copy link

codecov bot commented Oct 24, 2018

Codecov Report

Merging #32 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #32   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines          85     85           
  Branches       18     18           
=====================================
  Hits           85     85
Impacted Files Coverage Δ
src/FirestoreProvider.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dafa65a...56e6044. Read the comment docs.

@green-arrow
Copy link
Owner

@CreativeGuy2013 - Thanks for taking the time to open a PR for this! Since this is opting into potentially breaking functionality, I would rather have the user pass in a prop that explicitly opts into this new behavior.

Perhaps we could add a prop called useTimestampsInSnapshots that defaults to false, and pass that to the firestore initialization.

Would you be open to updating this PR with that and adding a bit in the README in the FirestoreProvider section about the new prop?

Thanks again!

@lucacasonato
Copy link
Contributor Author

Everything should work using useTimestampsInSnapshots. The default is false.

@green-arrow green-arrow merged commit c5dbbbc into green-arrow:master Nov 12, 2018
@green-arrow
Copy link
Owner

@CreativeGuy2013 - Thanks again for the PR and for the quick updates on this!

green-arrow pushed a commit that referenced this pull request Nov 21, 2018
…d of settings. (#35)

There was a bug introduced with #32 where the result of `firestore().settings(...)` was being returned, causing all `react-firestore` components to break since the result is not the firestore database instance.

Fixes #34
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.

None yet

2 participants