Object mutation and README vs firestore.rules

[fabyeah]

This looks like a great package, many thanks! 👍

I'm currently setting it up and ran into two issues:

1) Are you mutating the object passed to geocollection.add? I'm guessing so, as you are adding the g key. Can this not be done without mutation, as it is a wrapper I presume? Because I'm getting unexpected behavior with the object i'm passing, namely the sudden appearance of the g key.

2) I didn't see the firestore.rules file at first. As the README only mentioned the following:

interface GeoDocumentData {
  g: {
    geohash: string;
    geopoint: GeoPoint;
  };
  [field: string]: any;
  }

I adjusted my security rules accordingly, assuming the coordinates would be stripped (it's duplicate data). Is this field used for anything? If not, can this not be removed at the same time that g is added? Would be very helpful if the README was more clear about the necessary changes to the security rules.

[MichaelSolati]

Yes, the library adds a g field which stores all of the necessary data for geofirestore to execute a geoquery. Though I believe that you're suggesting that the library should clone the object before adding the field to it.. Which makes sense... However I'd have to move this issue the geofirestore-core which actually handles the encoding.

Anyway the library doesn't aim to modify our touch any of your data, only add a field that stores the data it needs. I'm not sure what wording or how security rule documentation would look based on your feedback, so I'd be open to a PR if you wanted to contribute that.

[MichaelSolati]

Just pushed up a fix, it'll be deployed over the weekend.

[fabyeah]

OK. Don't know how your fix is going to look, but I think if in the Data Structure section of the README the coordinates field is added and also a note that security rules should be extended with

&& request.resource.data.g.size() == 2
&& request.resource.data.g.geohash is string
&& request.resource.data.g.geopoint is latlng
&& request.resource.data.coordinates is latlng

that should be sufficient.