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

api: no apparent way of removing a specific .on() listener #713

Closed
leonardpauli opened this issue Feb 27, 2019 · 1 comment
Closed

api: no apparent way of removing a specific .on() listener #713

leonardpauli opened this issue Feb 27, 2019 · 1 comment

Comments

@leonardpauli
Copy link

TL;DR: no apparent way of removing .on() listener

  • .off() removes all listeners
  • can't find any other way in docs (for .on())
  • .get(()=> ...) is said to be "more low level", but has promising ev.off()
  • to be able to add/remove multiple listeners on same node is a very common pattern (eg. username both in header and profile card)
    • to use a custom cache with a single .on() works but seems like "duplicating the effort"
    • would be great if .on() gave the ability to disable that specific listener
    • eg. similar to id = setTimeout(...); clearTimeout(id)
    • eg. ctx = gun.get('a').on(...); ctx.off()
  • improper listener removal usually leads to "seemingly random" bugs

After digging in the code, .on() is more or less a wrapper for .get(), and passes along relevant args. See line 46. It would be nice to have that in the docs if it's the recommended way of removing specific listeners. Though having the ability to be calling .off() on the return value from .on() seems preferable in many ways (ability to remove listener before its first invocation, imho, "logical" syntax, etc).

This shouldn't require too much change (<10 lines?) if I saw correct (.on -> .get -> Gun.on that returns ev directly and has access to node, therefore adding ref to ev on node + make .off use node.ev.off() if ev exists, otherwise doing the usual).

Could make a PR if this behavior is the expected one.

r = gun.get('root')
a = r.get('anna')
a.put({age: 5})

// listener 1, eg. in header component
l1handler = (value, key)=> console.log(`1: a.${key} is now ${value} (header)`)
l1 = a.map().on(l1handler)

// listener 2, eg. in profile card component
l2handler = (value, key)=> console.log(`2: a.${key} is now ${value} (profile)`)
l2base = a.map()
l2ev = null // awkward to get hold of
l2handlerWrapper = (value, key, _msg, ev)=> {l2ev = ev; l2handler(value, key)}
l2 = l2base.on(l2handlerWrapper)

console.log('updating...')
a.get('age').put(6)

// leaving profile page, profile card component is destroyed, listener 2 should be removed
console.log('removing listener 2')
// l2.off() // no relevant effect
// l2base.off() // no relevant effect
// a.off() // removes both listeners
l2ev.off() // removes only listener 2 (what I'm looking for, but awkward as it is)

console.log('updating...')
a.get('age').put(7)

console.log('done')

/* log:
1: a.age is now 5 (header)
2: a.age is now 5 (profile)
updating...
1: a.age is now 6 (header)
2: a.age is now 6 (profile)
removing listener 2
updating...
1: a.age is now 7 (header)
done
*/
@amark
Copy link
Owner

amark commented Mar 26, 2019

@leonardpauli sorry this has taken me forever, I've been gone on international trips and super stressed out over RAD fixes and headed into launching this new thing soon.

You've done your research! Quite good work.

You are correct that .get args get passed through to .on(function(data, key, msg, eve){ eve.off() would yes indeed let you remove that specific listener.

Would you do me a favor and update the docs? They are wiki/editable at https://github.com/amark/gun/wiki/API which will update the live site.

Your idea of having gun.get('foo').on(cb).off() remove that last listener is quite good, but resulted in some problems when I tried developing it years ago:

  1. Performance. It meant every .on(cb would have to return a different return value that has unique context to the cb. This destroys the ability to cache the gun/reference chain/context and return it.
  2. Multiple? If you add multiple listeners, which one would it be turning off? As a result, it made sense that gun.get('foo').off() should turn off all listeners - which isn't always the intended behavior, but "safer".

Which is why I was left with leaving it to .on(function(data, key, msg, eve){ eve.off() to turn off the specific listener.

You are right this can lead to awkward situations. How do you turn it off before the callback fires (like canceling an event?).

This is a good question, but I do want to note that for now (API stability, not causing breaking changes, etc.) we're going to have to say eve.off() is the official answer.

But to address your points, I think we should consider:

  1. If you look at lib/open.js code, you'll see it has a lightweight wrapper around tracking a bunch of listeners, and then provides a uniform interface for unsubscribing. I equally imagine anyone adding a module for GUN that provides a framework around event handling, by building ontop of GUN's lower level features. This way, GUN doesn't get bloated with a lot of "management" code in core, but this event-handling framework could be bundled with something like JoyDB @bugs181 for most users starting out.
  2. I've also intended to make gun.get('foo').off() be more powerful/expressive, like gun.get('foo').off(true) would unsubscribe everything at current level (currently .off() is aggressive and unsubscribes everything at & beneath it), or you could pass it jQuery-style event handling, var myListener = function(){...}; ...; gun.get('foo').on(myListener); ...; ...; gun.get('foo').off(myListener) which would solve the edge case you bring up. Or other things like gun.get('foo').off(-1) unsubscribe the LAST added listener, or 1 for the FIRST added listener, or other jQuery-style even thandling like adding class/tags to listeners gun.get('foo').on(cb, {tag: 'currentPage'}) then gun.get('foo').off('currentPage') unsubs all listeners tagged that way.

Thoughts?

For now, gonna say eve.off() is the official answer tho (for API stability/no-breaking code) but this is certainly a good thread. I'll probably close it though, since GitHub is starting to report GUN as having "100 Issues" when most of these are questions/features, not actual bugs, and I don't want people thinking "wow that is a lot of issues". So can I close this, since it isn't a bug? I want the conversation to continue, but probably StackOverflow or https://github.com/gundb/feature-requests ? Closing, thanks!

@amark amark closed this as completed Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants