aleclarson / emitter-kit

Type-safe event handling for Swift
MIT License
567 stars 71 forks source link

fixing nil listening issue with Notifier #42

Closed markshiz closed 7 years ago

aleclarson commented 7 years ago

Nice catch! Any ideas on how we can support listening to notifs where notif.object must be nil (the current behavior)? It's an edge case, but maybe there's a way to keep that functionality without introducing a breaking change?

We could add a boolean property that allows your suggested functionality (listening to all notifs when the target is nil).

listener = event.on { (userInfo: NSDictionary) in
  // Since `strict` is false, the value of `notif.object` is not checked.
}
listener.strict = false

Or we could just offload that responsibility by passing notif.object as the second param to the listener.

listener = event.on { (userInfo: NSDictionary, object: Any?) in
  if (object == nil) {
    // This achieves the original functionality.
  }
}

I'm leaning towards the second one, since it's cleaner. It's a breaking change for anyone listening to a notif where notif.object might be nil or non-nil, but that's probably a rare scenario.

markshiz commented 7 years ago

Yes, I like the latter suggestion. One other possibility would be to send a NSNotification object in the callback. Both the object and userInfo are available from that object.

aleclarson commented 7 years ago

Good point, I'll make that change in another commit.

Before I merge this PR, you'll need to fix one thing.

When the NotificationListener has a "target" and notif.object is nil, the listener should not be triggered.

Here's the updated code:

if self._targetID != "0" {
  if let target: AnyObject = notif.object as AnyObject? {
    if self._targetID != getHash(target) {
      return
    }
  } else {
    return
  }
}
markshiz commented 7 years ago

LGTM

aleclarson commented 7 years ago

Merged into v5.1.0 🎉

Thanks for your contribution!