MichaelSolati / geofirestore-js

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

geofirestore near method returns no results when radius is negated or 0 #178

Closed francisleigh closed 4 years ago

francisleigh commented 4 years ago

I left a comment on this issue but i just wanted to make sure this issue is seen

radius: 0 returns all results whereas any other number returns none.

import { useFirestore } from "react-redux-firebase";
import { GeoFirestore } from "geofirestore";
...

const firestore = useFirestore();
const geofirestore = new GeoFirestore(firestore);
const geocollection = geofirestore.collection("shops");
const locations = [
    {
        latitude: 50.819224,
        longitude: -0.122760,
        name: "Barber Blacksheep",
    },
    {
        latitude: 50.819583,
        longitude: -0.124165,
        name: "Sharp Scissors",
    },
    {
        latitude: 50.819217,
        longitude: -0.124777,
        name: "Zar's Barbers",
    },
    {
        latitude: 50.830493,
        longitude: -0.147061,
        name: "Teddy Edwards",
    },
    {
        latitude: 40.7589,
        longitude: -73.9851,
        name: "TEST",
    },
];

useEffect(() => {
    locations.forEach((location) => {
        geocollection.add({
            name: location.name,
            coordinates: new firestore.GeoPoint(location.longitude, location.latitude),
        });
    });

    const query = geocollection.near({
        center: new firestore.GeoPoint(50.818398, -0.12713671),
        radius: 0,
    }).get().then((value) => {
        console.log(value.docs.map((d) => d.data()));
    });
}, []);

For the sake of this example i've put the query in the same useEffect as the geocollection.add but i can assure you that the collection is indeed populated before calling geocollection.near If i were to put the radius: 1000000 it yields no "shops" where 0 yields all 5

I have also tried the onSnapshot approach and get the same results

Screenshot 2020-06-07 at 16 56 32

^ Example of the data structure generated by geocollection.add

MichaelSolati commented 4 years ago

Hey @FrancisLeigh I appreciate the breakdown. I'm trying to replicate this, but I'm not at all familiar with react-redux-firebase, is there any chance you can create a stackblitz for me?

You can use this template with a sandbox firebase project

francisleigh commented 4 years ago

@MichaelSolati I have raised a separate query with react-redux-firebase to see if they have any knowledge on using your library with theirs. In the meantime i have created a reproducible demo on your sandbox without using react-redux-firebase and i still find myself having the same issues.

Please use the code below.

import * as firebase from 'firebase/app';
import 'firebase/firestore';
import { GeoFirestore } from 'geofirestore';

/**
 * Initialize Firebase
 *
 * NOTE: This will be publically visible
 * please don't accidentally publish your
 * actual production credentials here.
 * Create a sample project that mimics
 * your setup to reproduce the error.
 */
firebase.initializeApp({
  apiKey: 'AIzaSyCh4csJpj4JLXk0geXD7CPC_x-KO7KWXJY',
  authDomain: 'geofirestore-issue-sandbox.firebaseapp.com',
  databaseURL: 'https://geofirestore-issue-sandbox.firebaseio.com',
  projectId: 'geofirestore-issue-sandbox',
  storageBucket: 'geofirestore-issue-sandbox.appspot.com',
  messagingSenderId: '148423038085'
});

const geofirestore = new GeoFirestore(firebase.firestore());
const geocollection = geofirestore.collection("shops");

const locations = [
    {
        geohash: "gcpchgktj",
        latitude: 50.819224,
        longitude: -0.122760,
        name: "Barber Blacksheep",
        summary: "A classic welcoming Barbers",
        listings: [{ chair_rent: 50, average_day_taking: 200 }],
    },
    {
        geohash: "gcpchgktj",
        latitude: 50.819583,
        longitude: -0.124165,
        name: "Sharp Scissors",
        summary: "A Turkish barbering experience",
    },
    {
        geohash: "gcpchke9e",
        latitude: 50.819217,
        longitude: -0.124777,
        name: "Zar's Barbers",
        summary: "Nice small barbershop, 1-2-1",
    },
    {
        geohash: "djfnjrhjk",
        latitude: 50.829618,
        longitude: -0.147071,
        name: "Teddy Edwards",
        summary: "Sick cuts! love dan.",
        listings: [{ chair_rent: 50, average_day_taking: 200 }],
    },
    {
        geohash: "djfnjrhjk",
        latitude: 40.7589,
        longitude: -73.9851,
        name: "TEST",
        summary: "TESTERRRR",
    },
];
/** 
 * Un comment the lines below 'once to populate the DB and then comment back out to avoid
 * duplicate entries.
**/
// locations.forEach((l) => {
//     geocollection.add({
//         name: l.name,
//         coordinates: new firebase.firestore.GeoPoint(l.longitude, l.latitude),
//     });
// });

const query = geocollection.near({
    center: new firebase.firestore.GeoPoint(50.818398, -0.12713671),
    radius: 0,
})

query
.onSnapshot((snapshot) => {
  let markers = [];
  snapshot.forEach(doc => {
    markers.push({id: doc.id, ...doc.data()});
  });

  console.log("values from onsSnapshot", markers);
});

query
.get()
.then((value) => {
  console.log("Values from get", value.docs.map((d) => d.data()));
});

Once again with a radius: 0 i get all entries back and with radius: any_other_number i get no entries whatsoever.

MichaelSolati commented 4 years ago

This was cause by an inconsistency in how radius' were handled. In some places I didn't properly check to see if the value was a number, just if it existed. As a result 0 would be treated as falsey. v3.6.0 should fix this and is going live now!

francisleigh commented 4 years ago

@MichaelSolati Hi Michael, I appreciate you looking into this so swiftly.

I have updated the sandbox with version v3.6.0 and radius: 0 now does not return all results. I am however still experiencing the other side of the issue which maybe wasn't detailed enough in this ticket.

The real issue (for me) was that any other number that wasn't 0 yielded no results. The latitudes & longitudes that i have listed in my example code are within less than 3 miles of my current location (the .near query parameters) yet i can't get them back from the DB even with radius: 100000

Are you able to shine some light on this? Again i've used essentially the same code above to reproduce this issue on the sandbox with version3.6.0 installed.

Thank you

P.S if you would rather i create a new issue detailing this side of the initial issue i'm more than happy to.

MichaelSolati commented 4 years ago

Ok, so I went a little crazy looking at your code until I realized this...

/** 
 * Un comment the lines below 'once to populate the DB and then comment back out to avoid
 * duplicate entries.
**/
// locations.forEach((l) => {
//     geocollection.add({
//         name: l.name,
//         coordinates: new firebase.firestore.GeoPoint(l.longitude, l.latitude),
//     });
// });

GeoPoints take the argument in order of lat then lng, not lng lat. Flip the order and you're all set.

francisleigh commented 4 years ago

@MichaelSolati Yes! Nail on the head! thank you so much! And thanks again for your swift responses. Apologies for the sneaky semantics issue.