EddyVerbruggen / nativescript-plugin-firebase

:fire: NativeScript plugin for Firebase
https://firebase.google.com
MIT License
1.01k stars 444 forks source link

Nested GeoPoint gets saved as object in Firestore on iOS #947

Closed abhayastudios closed 6 years ago

abhayastudios commented 6 years ago

GeoPoint gets saved as object in Firestore on iOS with plugin 7.1.2 (and 6.8.1 for that matter). On Android it works fine.

EddyVerbruggen commented 6 years ago

Hi, can you share the call you use?

abhayastudios commented 6 years ago

Sure! It is a bit simplified but along these lines:

import { firestore } from "nativescript-plugin-firebase"; // firestore typings for GeoPoint
import * as firebaseWebAPI from "nativescript-plugin-firebase/app";
import * as geolocation from "nativescript-geolocation"; // plugin for getting device GPS coords

[...]

  private profilesRef:firestore.CollectionReference = firebaseWebAPI.firestore().collection("profiles");

  geolocation.getCurrentLocation({
    desiredAccuracy: Accuracy.any,
    iosAllowsBackgroundLocationUpdates: false,
    maximumAge: 5000, // in milliseconds
    timeout: 5000, // in milliseconds
  })
  .then((location) => {
    this.profile.gpsLocation = firestore.GeoPoint(location.latitude, location.longitude);

    this.profilesRef.doc(this.profile.uid).set(this.profile)
      .then(() => { console.log("Saved profile data to backend!"); })
      .catch((error) => console.log(`Error saving profile data to backend: ${error}`));
  }, (error) => { console.log(error); });

Also tried replacing setting the GeoPoint by below but with same result:

this.profile.gpsLocation = firebaseWebAPI.firestore().GeoPoint(location.latitude, location.longitude);
abhayastudios commented 6 years ago

@EddyVerbruggen let me know if you were able to reproduce or whether you need me to create a little app to demonstrate the issue

EddyVerbruggen commented 6 years ago

That's strange.. I just tested this in the demo-ng app (iOS sim, latest plugin version):

import { firestore } from "nativescript-plugin-firebase";
const firebase = require("nativescript-plugin-firebase/app");

  lastKnownLocation: firebase.firestore().GeoPoint(4.34, 5.67),
  lastKnownLocation2: firestore.GeoPoint(4.34, 5.67)

Result:

screenshot 2018-10-08 at 17 28 06

I will try .set as well.

EddyVerbruggen commented 6 years ago

Hmm, .set works equally well for me with similar code.

abhayastudios commented 6 years ago

Thanks for checking @EddyVerbruggen. I will try to replicate in a minimal project.

abhayastudios commented 6 years ago

Hi @EddyVerbruggen,

So it seems that the issue occurs only when using GeoPoint nested in an object, such as below.

let profile:any = {
      first: 'First',
      last: 'Last',
      location: {
        geoPoint: firestore.GeoPoint(52.345342, 4.900721400000066),
        latitude: 52.345342,
        longitude: 4.900721400000066
      }
    }

    let geoRef:firestore.CollectionReference = firestore.collection("geopoints");

    geoRef.doc('issue947').set(profile)
    .then(() => { console.log("Saved GeoLocation data to Firestore!"); })
    .catch((error) => console.log(`Error saving GeoLocation data to Firestore: ${error}`));
  }

Is this expected behavior? As said, on Android this still produces a GeoPoint type in Firestore.

Thanks!

EddyVerbruggen commented 6 years ago

That could indeed be a limitation, but let's see if it's fixable. Thanks for investigating this!

EddyVerbruggen commented 6 years ago

Fixed in 7.1.4. Note that on iOS the same issue/fix applies for storing nested server timestamps, document references, and array union/remove.

abhayastudios commented 6 years ago

@EddyVerbruggen thanks for the fix!

I was just having a go at it myself. Won't your fix have an issue with plain arrays as typeof [] === "object" will yield true? So something like this will prevent this:

} else if (typeof item === 'object' && item instanceof Object && !(item instanceof Array)) {
   return fixSpecialFields(item);
}
EddyVerbruggen commented 6 years ago

Hmm 🤔.. I don't have that kind of usage, but wouldn't mind merging that!

abhayastudios commented 6 years ago

OK submitted PR #951. Sorry, next time I will inform you when I decide to try myself :)

EddyVerbruggen commented 6 years ago

Haha no worries. I’ll pop out a new version late(r) today.