MichaelSolati / geofirestore-js

Location-based querying and filtering using Firebase Firestore.
https://geofirestore.com
MIT License
505 stars 58 forks source link

How to add document without the coordinates key? #84

Closed Annihil closed 5 years ago

Annihil commented 5 years ago

When adding a document to a geocollection like this

geocollection.add({
    xxx: 'yyy',
    coordinates: new firebase.firestore.GeoPoint(51.5108272, -0.0770001)
});

Since the GeoDocument structure is the following

interface GeoDocument {
    g: string;
    l: GeoPoint;
    d: DocumentData;
}

l and coordinates are redondant, is it possible to delete coordinates key of the document while adding it to the collection, or add another parameter to the add function to pass a GeoPoint to specify the coordinates?

MichaelSolati commented 5 years ago

So the redundancy is necessary. We need a GeoPoint in order to encode a geohash, which we query, and we use the GeoPoint for some calculation and validation under the hood (as geohashes aren't accurate). Ok, so we have explained why it's necessary at the top level.

Now going into the d level, that's your data, and you'll likely need to have the GeoPoint as a reference to the location. Now we could just pass back the l field, however we don't want to really touch any of your data in the d field. Also, the library allows you to mark any field as the GeoPoint, not just the coordinates field. That can be used as the second argument of the add method.

So with these considerations in mind this was the solution we/I opted for.

Annihil commented 5 years ago

While converting the document, why not deleting the coordinates key thought? It's not needed anymore, maybe we could have an option to remove it (not enabled by default)

MichaelSolati commented 5 years ago

We don't want to touch the users data though. There also isn't any real benefit to this (I'm not sure why this would be a feature)

Annihil commented 5 years ago
geocollection.add({
    xxx: 'yyy'
},
{lat: 51.5108272, lng: -0.0770001});

would be cleaner I believe

MichaelSolati commented 5 years ago

I don't think this would be the right direction for the library, and I can not think of a good reason to support this. However with transactions going to be supported in the very near future you could handle this with a simple function like this...

export class StripDocument {
  constructor(private _db: GeoCollectionReference) {}

  public add(doc: any, coordinates: firebase.firestore.GeoPoint) {
    const ref = this._db.doc();
    return this._db.firestore.runTransaction((transaction: firebase.firestore.Transaction) => {
      const geotransaction = new GeoTransaction(transaction);
      return geotransaction.get(ref).then(() => {
        geotransaction.set(ref, {...doc, coordinates});
        transaction.set(ref['_document'] as firebase.firestore.DocumentReference, doc);
      });
    });
  }
}

As mentioned GeoTransactions aren't out yet, hopefully by the end of the weekend. But this class/function should work for you when it is out (I didn't test it, I just threw it together quickly... It may need some tweaking)

(@Annihil I updated the code because of a mistake I realized I kept making when writing my tests. Version 3.2.0 is having tests run on travis and if everything goes well will be deployed on NPM in the next 15 minutes)

georgeMorales commented 4 years ago

So would it be possible to remove or not add coordinates in documentData? I want to have the least possible data in documentData and since in the geopoint we have the coordinates, would it be possible?

fabyeah commented 4 years ago

I have the same issue. Didn't see this when I created mine #181.