MichaelSolati / geofirestore-core

Lightweight location-based querying and encoding of Firebase Firestore Documents for geofirestore.
https://core.geofirestore.com
MIT License
3 stars 9 forks source link

generateGeoQueryDocumentSnapshotI() returns ...Data instead of ...Snapshot #6

Open spoxies opened 2 years ago

spoxies commented 2 years ago

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

Methods

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

spoxies commented 2 years ago

So the comment above was not valid, the snapshot was not exposed with that change. The manifestation of the resulting problem is that the/a implementing Geofirestore.GeoQuery.get() returns that has a method GeoQuerySnapshot.forEach (or property .docs), that passes the <QueryDocumentSnapshot>.

Of that <QueryDocumentSnapshot> the docs say: "The document is guaranteed to exist and its data can be extracted with .data() or .get() to get a specific field. A QueryDocumentSnapshot offers the same API surface as a DocumentSnapshot." ..and this is/was not true, as for example .get(<field>) is/was not exposed.

In a new commit I added the missing method(s)/property to utils.ts:generateGeoQueryDocumentSnapshot() of the geofirestore-core in a not so pretty way. I did notice that these missing methods are present in the src/GeoDocumentSnapshot.ts:class GeoDocumentSnapshot{} of geofirestore-js, but in my experience this did not expose them (at least within a cloud functions implementation) with a one time query near(..).where(..).get().

I think I do understand what the envisioned design/abstraction would be, but I was unsuccessful in implementing this in such a way that the geofirestore-core would stay untouched. As said earlier this could be a shortcoming of my understanding/ability with TS.

If indeed the geofirestore-core should be patched, then I'll try to tidy up the commit and create a PR, but the general idea is something for the creator @MichaelSolati to have a opinion about.