posva / vuexfire

Check
https://github.com/vuejs/vuefire
MIT License
558 stars 49 forks source link

Firestore timestamp change breaks vuexfire #161

Closed Tattomoosa closed 6 years ago

Tattomoosa commented 6 years ago

A few days ago, Firestore started sending a warning about an upcoming change to snapshots where they will be sent as timestamps instead of Dates. If you implement their recommended change to stop this warning then VuexFire breaks.

The Firestore warning:

[2018-04-22T20:01:39.948Z]  @firebase/firestore: Firestore (4.13.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.

The error:

vuexfire.es.js?b50a:338 Uncaught TypeError: ref.onSnapshot is not a function
at subscribeToDocument (vuexfire.es.js?b50a:338)
at eval (vuexfire.es.js?b50a:163)
at Array.forEach (<anonymous>)
at subscribeToRefs (vuexfire.es.js?b50a:148)
at Object.modified (vuexfire.es.js?b50a:235)
at eval (vuexfire.es.js?b50a:287)
at Array.forEach (<anonymous>)
at Object.eval [as next] (vuexfire.es.js?b50a:286)

I'm assuming this happens any time an object with a field that's a timestamp is received. Timestamps have a toDate() method so it should be an easy fix. I might look at when I get a chance but I figured I'd make an issue first.

posva commented 6 years ago

it should be an easy fix, indeed. thanks for letting me know 🙂

paulpv commented 6 years ago

I am seeing the following error when subscribing to a document w/ Timestamp fields:

Uncaught TypeError: ref.onSnapshot is not a function
    at subscribeToDocument (vuexfire.es.js?b50a:340)
    at eval (vuexfire.es.js?b50a:164)
    at Array.forEach (<anonymous>)
    at subscribeToRefs (vuexfire.es.js?b50a:148)
    at Object.added (vuexfire.es.js?b50a:210)
    at eval (vuexfire.es.js?b50a:288)
    at Array.forEach (<anonymous>)
    at Object.eval [as next] (vuexfire.es.js?b50a:287)
    at next (index.cjs.js?5a4f:16152)
    at eval (index.cjs.js?5a4f:14282)

The ref that is being passed in is normally a DocumentReference class...
...but when a document contains a Timestamp field the ref is actually a Timestamp class!

I think something in extractRefs is getting confused when it sees an object of type Timestamp.

paulpv commented 6 years ago

I think my theory was correct. I changed utils.js to have the following:

const isObject = o => o && typeof o === 'object'
// Quick and dirty; please improve per https://stackoverflow.com/a/332429/252308
const isTimestamp = o => o && o.constructor.name === 'Timestamp'

export function extractRefs (doc, oldDoc, path = '', result = [{}, {}]) {
  // must be set here because walkGet can return null or undefined
  oldDoc = oldDoc || {}
  const idDescriptor = Object.getOwnPropertyDescriptor(doc, 'id')
  if (idDescriptor && !idDescriptor.enumerable) {
    Object.defineProperty(result[0], 'id', idDescriptor)
  }
  return Object.keys(doc).reduce((tot, key) => {
    const ref = doc[key]
    // if it's a ref
    if (
      ref instanceof Date || isTimestamp(ref) ||
      ref == null ||
      (ref.longitude && ref.latitude) // GeoPoint
    ) {
      tot[0][key] = ref
    } else if (ref && typeof ref.isEqual === 'function') {
      tot[0][key] = oldDoc[key] || ref.path
      tot[1][path + key] = ref
    } else if (Array.isArray(ref)) {
      tot[0][key] = Array(ref.length).fill(null)
      extractRefs(ref, oldDoc[key], path + key + '.', [tot[0][key], tot[1]])
    } else if (isObject(ref)) {
      tot[0][key] = {}
      extractRefs(ref, oldDoc[key], path + key + '.', [tot[0][key], tot[1]])
    } else {
      tot[0][key] = ref
    }
    return tot
  }, result)
}
paulpv commented 6 years ago

Cool! Thanks!