Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Scope events / event bus #522

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
4 participants
Owner

IgorMinar commented Aug 17, 2011

this is still work in progress.. but feel free to provide feedback

Maybe $removeListener to match node. $removeOn doesn't read like a sentence.

ex. "$on event with name call listener with capture"

Owner

IgorMinar replied Aug 17, 2011

+1

Owner

IgorMinar commented Aug 17, 2011

you might notice that the $broadcast api uses different param passing style than $emit. I think that we should change the $emit api because of

  • varags are an api deathtrap - you can't extend the api once you use varargs
  • there is a major perf penalty for using apply + concat + slice as opposed to direct invocation: http://jsperf.com/fn-apply-vs-invoke

This is wrong because the event source always ends up being your parent scope as you resurse down on line 647.

You need $broadcast(name, message, opt_source) and then source: opt_source || this

Rather, the source (and currentTarget) is the current scope as you call each listener on line 637, not the parent. My bad. :)

Contributor

esprehn commented Aug 17, 2011

If perf is really a concern I'd suggest reusing the event object as well and not creating it again on every recursive call.

Better even the entire recursive phase can be written with a for loop instead. That might be desirable anyway. Your broadcast is depth first right now, breadth first might be better. Not actually sure which is correct.

Owner

IgorMinar commented Aug 18, 2011

yes.. the recursive traversal was just a lame initial implementation. I implemented loop-based traversal. It's depth-first (breadth-first would require a temp queue, so I went DF instead).

Contributor

esprehn commented Aug 18, 2011

Nice. I notice that $emit is still based on varargs, is the intent to rewrite it like $broadcast?

I also think the listener should be called against the scope you're listening on, not on the global scope.

listener.call(currentScope, event, message)

An alternative would be to add a context argument to $on as $on(type, listener, context). That aligns better with the "scope is an argument of the controller" future migration so I like it better.

.call(context || null, ...)

Owner

IgorMinar commented Aug 18, 2011

yes, once the dust settles $emit and $broadcast will use the same api style.

we are moving away from call/apply everywhere because it's hard to tell what "this" is in a given context. I'm actually thinking about passing scope into the listener just like we do in watch (and not exposing it via currentScope event param).

so you'd get:

function Ctrl() {
  this.$on('foo', function(scope, event) {
  }
}

but that will be unnecessary once we start passing scope into controllers:

function Ctrl(scope) {
  scope.$on('foo', function(event) {
  });
}

I am wondering what is the use-case for $emit - which goes only and always up to the root?

For me, the event system was about being able to decouple controllers, e.g. to get rid of circular dependencies or just to inform everyone, using application-wide event names, that something has happened. Such a case would be achieved by broadcasting from root:
scope.$root.$broadcast('application-wide-eventName'.....)

I can also imagine a case when I would like to broadcast starting from current scope, to make sure other parts of the application won't be notified (also performance gain is huge). That is also a decouple case, because such the events would never "leak" to other parts, which could, by accident, use same event names for their private things.
scope.$broadcast('sub_module-wide-eventName',...)

What is the $emit use-case then?

Owner

IgorMinar commented Aug 19, 2011

forms and mainly form validation.

I can see one problem with scope.$root.$broadcast - the event.target will point to the root scope instead of the one which actually triggered the event. Something like scope.$broadcastEverywhere(...) (or something shorter :) could broadcast everywhere without lying about the source of the event. Speaking of which - why is the property called 'target'? Isn't it actually the 'source' of the event?

Owner

IgorMinar commented Aug 19, 2011

why do you think that knowing about the scope that triggered the broadcast is important?

I think that whenever the parent knows about it's children then bad things happen - the only exceptions are $digest (similar to $eval in the old scope) and event propagation.

the property is currently called sourceScope, see my additional commits.

Contributor

esprehn commented Aug 19, 2011

I don't think $broadcast was really intended to be used in a way that you'd want to know where the event came from. Usually things calling it will be services which are on the root scope (conceptually) so they'd call this.$broadcast or eventually scope.$broadcast to notify the app that something global has changed, like the location of the page, Online/Offline transitions, and page hidden events (background tab). These are things where you want the whole app to react, but since it's a global state change the source doesn't really matter.

I can't think of a good use case for wanting some middle level scope to $broadcast an event to the whole app. Did you have something specific in mind witoldsz?

Well, imagine a simple case where you have an on-line shop with a basket and you click "buy this" on some item. Controller broadcasts the event and it is caught somewhere by a basket which adds an item, refreshes the status etc... So broadcasting does not originate from within service, but of course one could create a "buyItem" service and launch an event from there... BTW: don't you think that global events could compete with simple services?

As to your questions about importance of event source - it was not me who created "target" property referencing an event source, so do not ask me why is that for :) However, since events are asynchronous, I can imagine how important it could be sometimes, to be able to track/debug what comes from where, that is why I would not get rid of that information.

Owner

IgorMinar commented Aug 23, 2011

witold, in case of the shopping app. can't the buy button broadcast using currentScope.$root.$broadcast?

@igor

As I said before, one can use scope.$root.$broadcast - the only problem is that such an event will not tell you the truth about who actually fired it, because it will always point at the root scope. As we have discussed here before, we are either not interested in event source - we should then get rid of that property or we should keep that property (i.e. for logs/debugging) and we should make every possible effort to make to set it up correctly.

Owner

IgorMinar commented Aug 24, 2011

I'm ok with removing both sourceScope and currentScope or alternatively
renaming sourceScope to targetScope?

The names are fine now, the only thing which bothers me is that once you invoke this:
scope.$root.$broadcast(...)
the event receivers will see the events originating from the root scope, which is not entirely the truth. There is no other way for any scope to broadcast events to everyone, having event.sourceScope pointing at them, because sourceScope will always point at the root scope.

If we would have something like:
scope.$broadcastFromRoot(...) <-- yes, this name is not perfect
then it would be possible to produce events with correct sourceScope property set up.

So the bottom line is:
scopeXYZ.$root.$broadcast(...) ---> event.sourceScope == $root
scopeXYZ.$broadcastFromRoot(...) ---> event.sourceScope == scopeXYZ
in bot cases events are delivered to the same group.

Owner

IgorMinar commented Aug 24, 2011

let's see how what we have works in practice. I'm still not entirely sure if it is a good idea to expose callee or any of the scopes at all. we need to experiment with this beast first.

thanks for the input witold

Owner

IgorMinar commented Aug 24, 2011

landed in master as 9dee936

@IgorMinar IgorMinar closed this Aug 24, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment