MichaelSolati / geofirestore-js

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

Adding more fields #1

Closed nicksav closed 6 years ago

nicksav commented 6 years ago

Guys, I am still trying to get my head around, but as I understand technically you can add more fields into stored obj? As you are using sorting and start At and End At in filetrs, could be good to save more fields and pass more filters inside. Am I correct?

MichaelSolati commented 6 years ago

Hey @nicksav the way geofirestore works (as it is modeled as geofire), is that the library creates an index where you can query keys based off of lat/lngs. There are no at filters as we use geohashing to create a single string which we are able to query on. The geofire library does not allow for customized objects, and so for this library to replicate geofire for users of firestore, it does not support custom objects/fields.

I am open to changing this though, and have some thoughts on how I would like to handle this, but am not ready to make that big of a change atm.

morrislaptop commented 6 years ago

I too would really like this feature, it's really inconvenient to make 100 extra requests to get the object data. I can see in chrome devtools that geofire is already pulling it down as well.

There were LOTS of requests on the geofire repo for this feature.

The best PR I could find was https://github.com/firebase/geofire-js/pull/45

Would you be open to merging in a PR for this repo based off this?

MichaelSolati commented 6 years ago

@morrislaptop as long as it passes tests I'd say that I'm open. I mean it was something I was looking to do over the next week or two (wanted to have this up by the end of the month), however it'll require some planning on my end.

Nexuist commented 6 years ago

Just wanted to say that I was planning on working on a PR that fixes this tonight and @morrislaptop just beat me to it. Great job! 🥇

MichaelSolati commented 6 years ago

Hey guys while I appreciate the PR I think I'm going to be writing this myself. In my head I have a vision/ideas where I want this to go... And it will be a hard departure from how geofire worked. But I think you guys will like it! I'll be pushing code tomorrow for y'all to peak into it. It should be good.

MichaelSolati commented 6 years ago

Aaand I realized it may be my fault the tests are failing, however there's still some big changes I want to make.

morrislaptop commented 6 years ago

Could you merge in with a feature version and make your changes in a major version?

MichaelSolati commented 6 years ago

@morrislaptop possibly, let me fix the tests and then see if it passes. If yes, then sure, but I'll have to get back to you on that by tomorrow night.

MichaelSolati commented 6 years ago

Because the tests are failing it's not going to auto deploy, I'll have to deploy version 1.1 sometime this week (be excited!)

morrislaptop commented 6 years ago

Sweet! I think this will get a lot of users over from realtime / geofire over here!

MichaelSolati commented 6 years ago

Version 1.1.1 is up on NPM and should work! (Make sure you don't install 1.1.0, it had a kinda major issue where it accidentally deleted the distribution code on postinstall 😅)

MvRemmerden commented 6 years ago

In 1.1.1 and 1.2.0 there was a setWithDocument() method, which I assume is what was being talked about here, right? But it got killed off in 2.0.0, was there any reason for it? It would solve the biggest problem I'm facing right now.

MichaelSolati commented 6 years ago

Hey @MvRemmerden, so v2.0.0 no longer needs a function to set with document because it only works with documents! It's no longer designed to be a secondary index for docs that we chained on that functionality to. Now when you add or set you pass in the document you want to store. The only requirement is that you have a Geopoint on the doc as the coordinate field (however you can call that something else if you look through the docs on the readme). Soooo you could do this.

geoFirestore.add({
    name: 'A place',
    details: {
        good: 'stuff'
    },
    coordinates: new firebase.firestore.GeoPoint(37.79, -122.41)
});
MvRemmerden commented 6 years ago

That's amazing, had totally not seen that in the readme, thanks for clarifying!

mikeybellissimo commented 5 years ago

I noticed that the above code makes it so the coordinates are stored both in the data as well as the coordinate attribute. Is there a way to add without making the coordinates be added to data as well aside from just deleting it immediately?

MichaelSolati commented 5 years ago

@Leuchapolo because we don't track the field you use to store the coordinates, we need to store it in order to do client side checks of the distance (to filter out places). Unfortunately it is needed.