michaelsolati / geofirestore-core Goto Github PK

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.

Currently the method generateGeoQueryDocumentSnapshot() returns a QueryDocumentSnapshot.
This only exposes a few methods or properties compared to a Firebase QueryDocumentSnapshot.

This means that what one can expect from either a generic Query or a GeoQuery is very different,
making passing results to generic functions a bit non unified (talking about geofirestore-js here).

With version v2.2.3 of geofirestore-js I had patched (but not PR'ed) theQueryDocumentSnapshot.
So it would expose QueryDocumentSnapshot methods, making it a drop-in replacement/addition.

As a sketch/conversation starter I did the same thing for the 5.0.0 version of geofirestore-core
This means that the implementing geofirestore-js would start to return the missing methods.

Now I say as a sketch, because I struggle with TypeScipt ( hence :any ),
and there are probably some semantics or reasons to take into account.

The commit does a few things.

Types

• It renames the type GeoFirestoreTypes.QueryDocumentSnapshot to GeoFirestoreTypes.QueryDocumentSnapshotData, as the type was more ...Data than ...Snapshot
• • for the method decodeGeoQueryDocumentSnapshotData(), type QueryDocumentSnapshotData is still semantically valid and used
• The previous, type GeoFirestoreTypes.QueryDocumentSnapshot is updated to extend the Firebase.QueryDocumentSnapshot type, (exposing all methods) and adding distance : number

Methods

• generateGeoQueryDocumentSnapshot() now returns the original snapshot methods, and properties, but overwriting data() with decoded.data() and adding {distance : decoded.distance}

I wonder if this would be a good addition or if there are reasons why (not) to return the original snapshots.

[REQUIRED] Describe your environment

• Operating System version: MacOS X Catalina 10.15.7
• Browser version: Chrome Version 86.0.4240.198 (Official Build) (x86_64
• Firebase library (firebase, firebase-admin, etc...) and version: firebase: 8.0.2, firebase-admin: 9.4.1
• geofirestore-core version: 4.1.2
• geofirestore version: 4.3.0

[REQUIRED] Describe the problem

Describe the bug

See error below:

❯ ./node_modules/.bin/tsc --watch
[4:08:38 PM] Starting compilation in watch mode...

node_modules/geofirestore-core/dist/definitions.d.ts:2:10 - error TS2614: Module '"../../firebase"' has no exported member 'firestore'. Did you mean to use 'import firestore from "../../firebase"' instead?

2 import { firestore as webfirestore } from 'firebase/app';
           ~~~~~~~~~

[4:08:43 PM] Found 1 error. Watching for file changes.

Steps to Reproduce

1. npm install geofirestore
2. Import geofirestore
3. Try to compile typescript

Relevant Code

import * as geofirestore from "geofirestore";