firebase / geofire-js

GeoFire for JavaScript - Realtime location queries with Firebase
MIT License
1.45k stars 345 forks source link

Canceling query in callback of query.on('key_entered', callback) results in error #87

Closed ZenPylon closed 8 years ago

ZenPylon commented 8 years ago

Hi there, I think I've run into a bug when canceling a query from within the key_entered callback when the query already has locations tracked. I've included a JS fiddle below - it triggers the debugger right before the error. Just step out and you'll end up in geofire.on(), and on line 1110 (geofire.js) in the next iteration of the for loop, locationDict is undefined.

_locationsTracked is being reset on the query.cancel() call. I think a solution might just be to check that _locationsTracked[key] still exists before trying to see if _locationsTracked[key].isInQuery is true.

For some reason, it's not displaying an error in the console, but stepping through definitely shows there's an error.

JS Fiddle

EDIT: to clarify, it's actually throwing an error in an angular app I'm running with a local bower install of geofire. It's using the following dependencies:

geofire#3.2.3 firebase#2.3.2 rsvp#3.1.0

if that helps...

jwngr commented 8 years ago

Thanks for reporting this to us. It does in fact look like a bug. I think your proposed solution is correct. We actually already have that check for undefined logic here, but just don't have it here. I'll put together a fix, try to get a test for it, and deploy a new version of the lib.

ZenPylon commented 8 years ago

Just a note - my possible solution is to just break from the loop rather than continue through the loop and check each key. My reasoning was that if the query is cancelled, none of the key_entered callbacks should be fired anyway. Not sure if that's accurate, but I'm using that for now!

Cheers.

jwngr commented 8 years ago

Both approaches should work. I put together the initial suggestion which works well enough :)

jwngr commented 8 years ago

Version 3.2.4 of GeoFire JS is out. Should fix your problem. Let me know if you are still running into issues!

ZenPylon commented 8 years ago

Awesome, thanks!