MichaelSolati / geofirestore-js

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

Feature Request : Firebase 'Functions' Compatibility and/or Observing changes to geofirestore docs #7

Closed invention7 closed 6 years ago

invention7 commented 6 years ago

Hey Michael,

This is a bit of a two-headed question with the same goal : Do you plan on geofirestore updating its query results as document content is updated?

e.g. case: on the backend, firebase 'functions' does some data manipulation to be updated on one to many geofirestore docs. In my client app, I want geofirestore to be able to see those updates as they occur and bring them in to the existing query results.

Obviously, this can be done with a redundant Observable of the geofirestore collection...

Another thought I had was doing a .setWithDocument() anytime firebase 'functions' needs to update the content. (Would geofirestore see this as a change?) This would seem a little more in-line with your project.

If this latter were the case, the challenge I have is in implementing geofirestore inside of 'functions'.
Unfortunately, while the constructor for geofirestore requires type "firebase.firestore.CollectionReference", firebase-admin's collection type "FirebaseFirestore.CollectionReference" isn't compatible with the constructor, nor does it seem it can be cast to the desired type...

This is getting a bit outside my knowledge-base : could an alternative constructor with the other type be easily integrated? Or do you have a thought that I could try in a fork?

Anyway, this is very much spit-balling. If I can clarify, or if this is getting way outside geofirestore's scope, shout!

Thanks.

MichaelSolati commented 6 years ago

So I may not have a clear grasp on your question, but let me answer where I plan on taking geofirestore and you can tell me if that answered your question (I think it will).

As it stands right now geofirestore sort of serves as an index to make geoqueries on, and provides the key of the document you want (which would be stored in a separate collection). While you do now have the option to store the document in geofirestore, it is not replicated by updates to the original document (if there is one). So while geofirestore does use the onSnapshot to listen for changes to documents, it listens to changes in IT'S collection, not the one it is indexing.

In version 2.x.x however, geofirestore will not be an index to a separate collection, it will be the entry point TO the collection. Effectively 2.x.x will wrap every document in a hash as well as the coordinates, and while you could do straight queries on the collection (all your data will be in a field called d, for document). That way going forward geofirestore is not just indexing the data, but managing it. I think it's the best way to go moving forward, and will complete the goal of making geoqueries feel more natural with Firestore. (I just need to implement code for adding new documents, update the docs, and possibly look at supporting more complex queries than just on location...)

invention7 commented 6 years ago

Got it. Yeah, I wasn't clear : I'm using your 1.2.0 which does have the .d object in which I'm placing all my data, so, yes, I'm already going down the route you're describing.

So then, making sure I'm understanding the 2.x vision : you're then looking at extending how geofirestore handles ".d", so that it serves as an entry point to all the data under the 'geofirestore' collection, which would include facilities to query and observe updates to the .d contents.

If I'm repeating you correctly : then, yeah, that's what I'm asking : )

... and whether you could add supporting firebase's cloud functions to 2.x

MichaelSolati commented 6 years ago

Soooo yes, I think we're on the same page.... However! Theoretically you should be able to use this already in cloud functions. Now there can be issues (see #5 ) however that is the nature of the package, because we use onSnapshot the queries stay active.... I am yet to see a better solution ATM...

Anyhoo, over the weekend I'm going to work on a personal project, where I can dig a little further into the firebase functions issue you're describing.

invention7 commented 6 years ago

:D Yeah, I think we're talking the same thing re : 2.x. (Do you have a wild-guess on a timeline?) re : Firebase Cloud Functions - - I was actually having trouble instantiating geofirestore (type mismatch passing a firebase-admin collection to the geofirestore constructor which expects firebase.firestore.collection). Anyway, I'll double check my code. I must be doing something funky.

Thanks for the quick replies.

invention7 commented 6 years ago

So, I'm cool in Cloud Functions. My mistake.

I was thinking if I updated the geofirestore record via .setWithDocument, maybe it would trigger a new key_entered the query, not so... : ) Again, I know I'm stretching the scope... if I wanted to nudge geofirestore to bring in the updated .document (e.g. the key is created, it has entered the query results, and then some process updates a value inside the .d object), what options would I have (... move the object to [0,0] and then back to its proper coordinates? : )

Thanks.

dramentol commented 6 years ago

Hey, @invention7 , I'm having the exact same problem running GeoFirestore in Cloud Functions. Can you expose what the solution was for you?

MichaelSolati commented 6 years ago

@invention7 i've been digging through code, I'll have better answers in the coming weeks, but i have a bigger focus atm to expand the ability to query firestore. I'll hopefully be able to get back to this soon.

MichaelSolati commented 6 years ago

Hey @invention7 so I was a little too busy to hit on this right away, but I've been thinking as to how/should I implement a feature like this... What will likely happen is I'll create a new on event, probably called key_changed so you can listen to changes to documents that are in your query. I mean I'm receiving that information, so I may as well do something with it. However IF I do do that, I will only emit values that are in the current query... If by the changing of the document the key_exited and key_moved events would be triggered, then we would not trigger key_changed (or maybe I'll call it key_modified...)

invention7 commented 6 years ago

Hey @MichaelSolati, Yeah! That sounds perfect : clean and simple.

stubborncoder commented 6 years ago

Hey invention7, did you finally manage to get this working with Functions? I have the same casting problem, and I'm kind of lost here.

invention7 commented 6 years ago

Hey @stubborncoder, yeah, I was new to cloud functions and thought I had to use the firebase-functions library; you can create a function with the defacto 'firebase' library and your collectionRef just as if it were a client app.

e.g. import * as firebase from 'firebase';

// define your firebaseConfig object

firebase.initializeApp(firebaseConfig); const collectionRef = firebase.firestore().collection('geofirestore'); const geoFirestore = new GeoFirestore(collectionRef);

If you want to kick this around some more, let's create a separate issue from this 'enhancement' thread.

MichaelSolati commented 6 years ago

@invention7 if you want to throw it into a separate error, I would appreciate that. (I'll be closing this hopefully early next week...)

invention7 commented 6 years ago

Suite, Michael! I'm excited to see.

I think @stubborncoder and I are done with our sidebar.
David, if you need to continue, we'll start a new issue.

Seth

stubborncoder commented 6 years ago

Yes thank you both!.

MichaelSolati commented 6 years ago

@invention7 I think (after exhausting my query limits on the spark tier running and writing tests), that i have the key_modified feature implemented. The docs have been (somewhat) updated (in the dev branch) to reflect that => https://github.com/MichaelSolati/geofirestore/tree/dev#geofirestorequeryoneventtype-callback (I say it exists, I didn't give an example... I'm open to PRs!!)

Anyway, version 2.0.0 will have a lot of changes under the hood, where I'd recommend reading the docs in the README. I also plan on writing a sample application so people can see how to use it.

If you want to try 2.0.0 right now, go for it! you can use it by npm install geofirestore@next

I'll close this after I build the demo app, confirm everything works as expected, and release 2.0.0 properly.

invention7 commented 6 years ago

WOOOOOOOOOOOO!!!!! I'll check it out today and get back. Thanks for the update!

invention7 commented 6 years ago

@MichaelSolati, I'll get back as I work with 2.0 some more, but on first blush : this is EXCELLENT! I've got the cloud functions I mentioned earlier making changes to the 'document', and those updates are flowing nicely to my client app under 'key_modified'. I also like the cleanup and consolidation of the methods (e.g. just 'set' vs 'set'/'setWithDocument'), standardizing on GeoPoint, etc Simplified and versatile! This is excellent!

MichaelSolati commented 6 years ago

Launched v2 properly, so I'm closing this issue! (Enjoy)