-
Notifications
You must be signed in to change notification settings - Fork 27.5k
feat($rootScope): adds $onRootScope method #5507
Conversation
This adds an $onRootScope method to the scope type. This allows developers to use $rootScope.$emit + $scope.$onRootScope as a fast eventBus which doesn't use scope bubbleing. Fixes angular#4574, Relates to angular#5371
@IgorMinar just to lower the barrier to include that for 1.3 I prepared this PR with tests included. The only thing I'm not 100% sure about is the documentation update (the wording and the link to StackOverflow) |
@@ -1116,6 +1116,72 @@ describe('Scope', function() { | |||
})); | |||
}); | |||
|
|||
describe('$onRootScope', function() { | |||
|
|||
it('should add listener for both $emit and $broadcast events that are triggered on $rootScope', inject(function($rootScope) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there isn't really a distinction between $emit and $broadcast events, so this confuses me... I don't think it's terrible to test both, but the name just seems a bit more verbose than it needs to be (it is just my opinion, though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you are right. I basically just copied over the tests for $on
to make the tests look more or less the same. I'm happy to change it if I should. But it should then be changed for the $on
tests as well.
There is one alternative to this change which we should consider. In AngularDart we implemented all watches and event listeners as a simple linear list and we keep metadata about the boundaries for the items in the list as demarcated by scopes (all watches/listeners for a given scope are grouped together). This means that when the listeners need to be called we just iterate over the list and call them in order and when a scope is destroyed we find the segment in the list and relink it. This gives you the same perf benefits as $onRootScope without the extra api and putting the responsibility on thinking about which api to use on the developer. |
Not sure I fully get the implementation yet but if what you propose can make such claims than I'm all for it:
If the above holds true for your proposed the solution than yep I'm all for it! |
I just wanted to let you guys know that I updated cburgdorf's jsperf and the problem is no longer there in the latest angular.js as of this writing (1.2.16) (probably fixed earlier). The jsperf was referencing an older Angular. Although the trick of using $rootScope.emit() is actually quite clever trick, it has side-effects, and is no longer necessary. The updated jsperf shows the problem is no longer there: http://jsperf.com/rootscope-emit-vs-rootscope-broadcast/27 By the way, when I say it has side-effects, what I mean is -- there are places where it's better to listen using $scope.on(). $that $scope object will go away if it's associated with a controller referenced from a view when the user changes pages. When the $scope is destroyed, anything it's listening on is unsubscribed. However, if every controller listens using $rootScope.$on(), then that subscription will stay in $rootScope, and will accumulate. You'll have to listen on the $destroy event of $scope and then manually unsubscribe using the original return value of $rootScope.on(). Thankfully, that type of processing is no longer necessary. |
@kasajian just a note here: I never suggested to use |
In the light of all the performance improvements we've done since this issue was created, does it still make sense to add an api like I'm thinking that the answer is no. Anyone disagrees? @cburgdorf? |
Nope, should be fine now. |
@IgorMinar could you describe what improvements affect this? |
Can someone please clarify this, originally we had this issue: #4574 However it was closed and "the discussion" was moved to this issue. Now this issue is closed but as far as I can tell from reading through it nothing has actually changed except that there might be some improvement in the original messaging system that will make it faster but I'm not sure if that relates to all the issues people were experiencing entirely such as unregistering being tricky etc. Many of us are using workarounds like making our own messaging service like this one: So what is now the guidance for the original problem of using Angulars event system as an event bus? Or is it in a state of flux and not actually recoded to address usage as an event bus and we should stick with our workarounds? |
@WhatFreshHellIsThis The way I see it you can now safely use |
var unsubscribe = this.$root.$on(name, listener); | ||
this.$on('$destroy', unsubscribe); | ||
|
||
return unsubscribe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we unsubscribe both listeners? root.$on
& this.$on
This adds an $onRootScope method to the
scope type. This allows developers to use
$rootScope.$emit + $scope.$onRootScope as
a fast eventBus which doesn't use scope
bubbleing.
Fixes #4574, Relates to #5371