Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref.limit() not working for updateCriteria() #26

Closed
MvRemmerden opened this issue Aug 8, 2018 · 7 comments
Closed

ref.limit() not working for updateCriteria() #26

MvRemmerden opened this issue Aug 8, 2018 · 7 comments
Labels
discussion wontfix This will not be worked on

Comments

@MvRemmerden
Copy link

Whenever I use ref.limit(x) in updateCriteria(), the actual maximum number of places returned is x+1.

@MichaelSolati
Copy link
Owner

@MvRemmerden this may be an issue specific to Firestore, however I also think this may be an issue in how queries work.

The geofirestore (and geofire) package wraps/scans hashes around the hash of what/where you select. That way if you're point is by the borderline of where a hash could be we don't miss areas right outside of the query. So if we're doing multiple queries in order to ensure that we have the full area covered then we will hit the ref.limit for each query (which will obvs be more than the initial x that you wanted).

I think in order to really fix this, the way queries are made will have to be completely rewritten... And still won't be perfect. Generally speaking, I don't think it can be done with a geohash strategy to queries... But I can give it a look/think... (Like how would the limit work if we're listening to changes?! Really unsure how to approach this)

@MichaelSolati MichaelSolati added help wanted Extra attention is needed wontfix This will not be worked on discussion labels Aug 8, 2018
@MvRemmerden
Copy link
Author

Very interesting, I will have to dive deeper into how the entire process works to actually help for a long-term solution.

For a quick fix, the easiest way might be to take the final array and remove items until we arrive at the number provided in the limit() method (or loop over the items and remove the ones furthest away from the center). That way the results would actually match the number that is expected.

@MichaelSolati
Copy link
Owner

Additional problem that I'd appreciate your thought on. So, while I may be able to do a limit on the first/initial query (which will slow down the query a little bit). The on_entered and other listeners return values as they come into the query zone. If a new value is entered into the zone then things get weird... Maybe the ready command could/should return the initial query with the points, and after that new values come in on the key_entered (however that could be problematic as well, I'm not sure how I would get the ref limit out of the query function you pass into the querycriteria)

@MvRemmerden
Copy link
Author

Good question. I think in most use cases there is a huge difference between what the data from the ready call and the data from on_entered will be used for. I would even go so far as to say that around 50% of use cases will never make use of the on_entered call.

Example: Searching for a place on Booking.com or Airbnb where you can display places to stay on a map. The only time the results get updated is when you move the map manually, and that's what updateCriteria is for. In this case it makes sense to return the initial query with the limit as not to create information overload.

And most other use cases I can imagine where on_entered is being used, that event of a new item popping up is different (and maybe even of another importance) in comparison to the initial display of data.

@MichaelSolati
Copy link
Owner

What you're describing as use cases makes sense, but it really takes away from the intended use case of this library (which is effectively to bring geofire to Firestore). I'm goin to leave this open, but I'm not sure how or even if I'll be addressing this.

(As a note I checked, I wouldn't be able to get the limit value that you would use in your query function, so I wouldn't be able to manually limit it on my end)

@MichaelSolati MichaelSolati removed the help wanted Extra attention is needed label Aug 10, 2018
@MvRemmerden
Copy link
Author

I certainly get that. As soon as I'm finished with my project, I can publish the source code and the workarounds I built to get these use cases to work. In some situations, it might not hurt to provide some additional data (as for the initial data in the ready callback), other situations might be solved by adding small helper methods, and for others some kind of best practice examples in the docs might be useful.

@MichaelSolati
Copy link
Owner

So I won't be "fixing" this, but I will make a note in the documentation about this issue for users moving forward. I will be closing this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants