return a listener+path from firebase._addObservers

[andrew19881123]

Hi Eddy, i want to propose you some little changes so we can use the removeEventListener

first we return an object with path and listener from the _addObservers

firebase._addObservers = function(to, updateCallback) {
   var listener = new com.google.firebase.database.ChildEventListener({
   [...]
   return listenerPlusPathObj;
}

and we resolve the promise with it

firebase.addChildEventListener = function (updateCallback, path) {
   [...]
   var listenerPlusPathObj = firebase._addObservers(firebase.instance.child(path), updateCallback);
   resolve(listenerPlusPathObj);
   [...]
}

we return the same obj here

firebase.addValueEventListener = function (updateCallback, path) {
   [...]
   resolve(listenerPlusPathObj);
   [...]
}

and then again we resolve the promise with the obj

firebase.query = function (updateCallback, path, options) {
   [...]
   else {
        var listenerPlusPathObj = firebase._addObservers(query, updateCallback);
        resolve(listenerPlusPathObj);
    }
   [...]
}

of course in the query we don't have to think about the singleEvent option because that listener should die after the first call.

I imagine the listenerPlusPathObj like

{
  "listener": listener,
  "path": path
}

in this way we can call removeEventListener(obj.listener, obj.path);

now we have all the tools to use the firebase.removeEventListener = function (listener, path) which is also very useful for my porting of geofire as a garbage collector for all the unused listeners.
what do you think? any problems or better ideas?

[EddyVerbruggen]

Great suggestion, I'm on it!

[EddyVerbruggen]

Btw would it be convenient/logical to indeed include the path in the result as well? Because you're already passing it in. Not that there's any harm including it of course.

[EddyVerbruggen]

Hi, I've added this feature to the master branch, but there's a caveat, see this issue in the Android Runtime repo I just created.

So if you only need iOS, or only one type of listener (value or child) then you're good. If you need to remove both types of listeners on Android then you'll run into this issue.

The TS definition has been updated according to this change, as well as the demo app so that should make clear how it works.

Not that on iOS the child event thingy actually creates 4 listeners so that's why I'm returning an array of listeners, and offer a removeEventListener as well as a removeEventListeners function.

Let me know what you think,
Eddy

[andrew19881123]

Hi, that's a great work! I've reviewed the code just now (not tested yet) and just one thing came to mind:
because the query() returns 1 or 4 listeners depending on the platform and one goal of this plugin is to create a layer of abstraction above the two, in this situation a developer has to understand the problem and use properly the two in the correct case, like removeEventListeners against the query() function (that's not obvious).
So my idea is to hide from the public api the removeEventListener and just give the developer the option to use removeEventListeners which takes an array of listeners (even an array with one element).
In this way it's not confusing, because you can choose only between one option, and you keep the same functionality...

[EddyVerbruggen]

@andrew19881123 That makes sense. I added the singular version for convenience but perhaps it's just only confusing folks.

[EddyVerbruggen]

OK, that's done.

[EddyVerbruggen]

Btw that NativeScript-Android issue has been fixed and the kind folks at NativeScript will ship the fix with the next release. So we made the framework better with this feature as well 👍

[EddyVerbruggen]

Note that {N} 2.3.0 is now released and indeed fixes this issue. Shiptime!