angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.86k stars 27.52k forks source link

Provide $scope.$onRootScope method #4574

Closed cburgdorf closed 10 years ago

cburgdorf commented 10 years ago

Many people are afraid of using angulars event mechanism as an EventBus as they fear of performance issues because of it's bubbling behavior.

However, those concerns don't apply if only $rootScope.$emit/ $rootScope.$on is used as there is no bubbling happening since $emit only bubbles upwards and there is no scope above the $rootScope.

I addressed this in my answer on the same thread.

In fact if eventing is used like that in an angular application than $rootScope is no different than the typical EventBus pattern. This comes with the drawback of manually having to unregister handlers from within controllers. Why that? Because controllers aren't singletons in Angular and therefore one needs to listen to the local $scope's $destroy event and then unregister from the event emitted by the $rootScope.

So controller code would look like this:

angular
    .module('MyApp')
    .controller('MyController', ['$scope', '$rootScope', function MyController($scope, $rootScope) {

            var unbind = $rootScope.$on('someComponent.someCrazyEvent', function(){
                console.log('foo');
            });

            $scope.$on('$destroy', unbind);
        }
    ]);

As stated in my SO post one can monkey patch the $rootScope to provide an alternative to it's $on method that takes an additional parameter with an $scope to listen for it's $destroy event to then do the deregistration for us.

 angular
    .module('MyApp')
    .config(['$provide', function($provide){
        $provide.decorator('$rootScope', ['$delegate', function($delegate){

            $delegate.$saveOn = function(name, listener, scope){
                var unsubscribe = $delegate.$on(name, listener);

                if (scope){
                    scope.$on('$destroy', unsubscribe);
                }
            };

            return $delegate;
        }]);
    }]);

With this in place we can subscribe to events emitted by the $rootScope and have them automatically deregistered when our local $scope is being destroyed.

We can use it like this:

angular
    .module('MyApp')
    .controller('MyController', ['$scope', '$rootScope', function MyController($scope, $rootScope) {

            $rootScope.$saveOn('someComponent.someCrazyEvent', function(){
                console.log('foo');
            }, $scope);
        }
    ]);

However, I think we could do much better by providing a $onRootScope method directly on the Scope type so that it's available on every $scope. This would then automatically make the deregistration for us but we wouldn't have to pass the $scope explicitly.

It would simply look like this.

angular
    .module('MyApp')
    .controller('MyController', ['$scope', function MyController($scope) {

            $scope.$onRootScope('someComponent.someCrazyEvent', function(){
                console.log('foo');
            });
        }
    ]);

As far as I know, I can't monkey patch that directly.

Shamelessly pulling @IgorMinar @mhevery @btford and @petebacondarwin and @matsko into this issue ;-)

petebacondarwin commented 10 years ago

I would rather provide a custom eventBus service that does this for us than adding new stuff to rootScope. This would be more self documenting. On 22 Oct 2013 09:46, "Christoph Burgdorf" notifications@github.com wrote:

Many people are afraid http://stackoverflow.com/a/12015847/288703 of using angulars event mechanism as an EventBus as they fear of performance issues http://stackoverflow.com/a/12015847/288703 because of it's bubbling behavior.

However, those concerns don't apply if only $rootScope.$emit/ $rootScope.$on is used as there is no bubbling happening since $emit only bubbles upwards and there is no scope above the $rootScope.

I addressed this in my answer on the same threadhttp://stackoverflow.com/a/19498009/288703 .

In fact if eventing is used like that in an angular application than $rootScope is no different than the typical EventBus pattern. This comes with the drawback of manually having to unregister handlers from within controllers. Why that? Because controllers aren't singletons in Angular and therefore one needs to listen to the local $scope's $destroy event and then unregister from the event emitted by the $rootScope.

So controller code would look like this:

angular .module('MyApp') .controller('MyController', ['$scope', '$rootScope', function MyController($scope, $rootScope) {

        var unbind = $rootScope.$on('someComponent.someCrazyEvent', function(){
            console.log('foo');
        });

        $scope.$on('$destroy', unbind);
    }
]);

As stated in my SO post one can monkey patch the $rootScope to provide an alternative to it's $on method that takes an additional parameter with an $scope to listen for it's $destroy event to then do the deregistration for us.

angular .module('MyApp') .config(['$provide', function($provide){ $provide.decorator('$rootScope', ['$delegate', function($delegate){

        $delegate.$saveOn = function(name, listener, scope){
            var unsubscribe = $delegate.$on(name, listener);

            if (scope){
                scope.$on('$destroy', unsubscribe);
            }
        };

        return $delegate;
    }]);
}]);

With this in place we can subscribe to events emitted by the $rootScopeand have them automatically deregistered when our local $scope is being destroyed.

We can use it like this:

angular .module('MyApp') .controller('MyController', ['$scope', '$rootScope', function MyController($scope, $rootScope) {

        $rootScope.$saveOn('someComponent.someCrazyEvent', function(){
            console.log('foo');
        }, $scope);
    }
]);

However, I think we could do much better by providing a $onRootScopemethod directly on the Scope type so that it's available on every $scope. This would then automatically make the deregistration for us but we wouldn't have to pass the $scope explicitly.

It would simply look like this.

angular .module('MyApp') .controller('MyController', ['$scope', function MyController($scope) {

        $scope.$onRootScope('someComponent.someCrazyEvent', function(){
            console.log('foo');
        });
    }
]);

Shamelessly pulling @IgorMinar https://github.com/IgorMinar @mheveryhttps://github.com/mhevery @btford https://github.com/btford and @petebacondarwinhttps://github.com/petebacondarwinand @matsko https://github.com/matsko into this issue ;-)

— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/4574 .

cburgdorf commented 10 years ago

@petebacondarwin I'm not really sure. Then you would have events in two different places and you would always have to decide whether you want it to be this way or that way. If you later on decide that an event should bubble down the scope hierarchy, you would have to move it from the eventBus to the $rootScope. On the other hand, I think the bubbling is only beneficial for events that are not application wide. I don't see a reason why things like $locationChangeSuccess should bubble down the scopes. It's an application wide event it should just be raised on the EventBus (whether it be $rootScope or $eventBus) and if you want to listen to it, you would subscribe just their on the EventBus.

The only benefit of the bubbling currently is that you don't have to care about cleaning up the handler as that's automatically done for you when your local $scope gets destroyed. However, that's exactly what would also be solved by my $scope.$onRootScope proposal.

So when you say you would rather have a seperate $eventBus implementation, would you agree that things like $locationChangeSuccess should rather go their so that they don't bubble anymore? If so, how would you handle the deregistration of handlers? If it's not something cleanly built in, people will create memory leaks all over the place. Or would you still use a $scope.$onEventBus then to have automatic deregistration when your local scope gets destroyed?

ajoslin commented 10 years ago

You can do the decorator more easily:

$provide.decorator('$rootScope', ['$delegate', function($rootScope) {
  $rootScope.prototype.$onRootScope = function(eventName, callback) {
    var unbind = $rootScope.$on(eventName, callback);
    this.$on('$destroy', unbind);
  });
});

The scopes will inherit the $onRootScope method, and now any scope can do $scope.$onRootScope().

I guess this would enhance performance in some situations ... but doesn't seem like a new method in core is the right way to do it.

cburgdorf commented 10 years ago

@ajoslin oh man, I didn't see the forest for the trees :) Great to see it's possible with the decorator. However, I really think this should be addressed in core. I think this should be the default for application wide events, no? Can you come up with any scenario where it's actually beneficial that $locationChangeSuccess does bubble down all the scopes? If it would just $emit on the $rootScope and everyone would just use $onRootScope to subscribe to such application wide events, we would all benefit from faster events without giving up on auto resource cleaning.

wesleycho commented 10 years ago

I've found in my experience that you should only be using $emit/$broadcast sparingly, since using it instead of $watch becomes harder to maintain (plus ends up being quite more verbose). My only good use case has been to handle when a particular piece of DOM has been rendered.

As for $locationChangeSuccess, it may be possible you may want to have your logic dependent on the $scope hierarchy, and potentially execute depending on the order it comes in if it isn't prevented somewhere along the tree.

Modifying the core of $rootScope to accomplish your purpose doesn't feel right though.

cburgdorf commented 10 years ago

@wesleycho I heard this claim before but I'm not ready to give up on event based architecture yet. We are in the process of building a SDK for www.couchcommerce.org. The SDK enables you to write web apps that run on our CouchCommerce platform. All of our core functionality is accessible by client side services that don't have any dependencies on Angular. We have an angular specific build though that wraps those services into modules and module.factory() to make them accessible with angular's DI.

Let's take something like a basketService. In it's current implementation it raises events like itemAdded, itemRemoved etc that you can subscribe to to invoke further actions or manipulate the UI etc. One of the core philosophies behind our SDK is that it works great with any framework (while explicitly providing further assistance if you pick angular as your framework of choice) may it be jquery-mobile, emberjs or AngularJS. Since not every framework has something like angular's $watch, raising events from within services to react up on from within controllers really works well for us.

Currently we raise events directly on the services their selves (our framework brings it's very lightweight own pub/sub mechanism) but we further plan to use a dedicated eventBus to raise events on. Since our whole library is built around the idea of dependency injection, we can simple inject the eventBus in all services which need to raise events. Thinking this a step further we could replace that eventBus with angular's $rootScope for the angular build of our SDK. So everyone who uses our SDK + angular gets angular's built in eventBus ($rootScope) whereas all others who choose something different (e.g. SDK + jquery-mobile) get our own implementation of the eventBus. Of course both implementations need to share the same interface or work through adapters though. However, this way our SDK doesn't feel alien to angular devs as all events will just be emitted on the $rootScope and furthermore they save bytes as we don't need to deliver our own eventBus implementation for those folks who use the SDK + angular.

This might be a very special use case but at least I hope i was able to provide some insights on why I'm not ready to give up on events as the main communication channel :)

cburgdorf commented 10 years ago

@ajoslin I had to roll back your edit in that SO post as $delegate.prototype isn't available to patch. That was why thought it wouldn't be possible to monkey patch at all.

cburgdorf commented 10 years ago

@ajoslin fixed it so that it now works together with isolated scopes, too. It has to be:

 angular
    .module('MyApp')
    .config(['$provide', function($provide){
        $provide.decorator('$rootScope', ['$delegate', function($delegate){

            $delegate.constructor.prototype.$onRootScope = function(name, listener){
                var unsubscribe = $delegate.$on(name, listener);
                this.$on('$destroy', unsubscribe);
            };

            return $delegate;
        }]);
    }]);
btford commented 10 years ago

Shamelessly pulling @IgorMinar @mhevery @btford and @petebacondarwin and @matsko into this issue ;-)

^ FYI, this tactic will not get your issue more attention (and if it did, soon everyone would be using this "trick" and it would quickly become useless). We do our best to look at each issue already. Please don't do this in the future.

cburgdorf commented 10 years ago

aye aye, sir!

What do you think about the issue itself? Considering how many CPU cycles are wasted by people using $rootScope.$broadcast + $scope.$on I don't see why we shouldn't give them an $onRootScope method so that they can replace that pattern with the faster $rootScope.$emit + $scope.$onRootScope pattern.

btford commented 10 years ago

Considering how many CPU cycles are wasted

I'd be curious to see a benchmark of the difference. I think a separate service might be better, but I'm not convinced that this is that much cheaper than broadcasting on $rootScope. I suspect a digest is the expensive part, and you aren't avoiding that with this approach.

cburgdorf commented 10 years ago

This very much depends on how many scopes you have on your page. I try to come up with a jsperf over the next days :)

cburgdorf commented 10 years ago

Hey @btford I put together a simple jsperf that compares the perf differences between $broadcast and $emit on a decent scenario with 100 scopes.

bildschirmfoto 2013-10-23 um 20 36 46

Here is the link to the perf

cburgdorf commented 10 years ago

Just do add another thought here. I know that event handling in Angular is somewhat designed around the idea of browser events. Just I wonder if global events like (window) load, hashchange etc bubble downwards through the entire DOM tree? As far as I know they don't.

You can fiddle with the hashchange event here on quirksmode for instance and it doesn't seem to bubble down. http://www.quirksmode.org/dom/events/tests/hashchange.html

I have a hard time finding use cases for $broadcast honestly. I understand that such fundamental things shouldn't be changed before 1.2 but I propose to change them afterwards. By promoting this $rootScope.$broadcast pattern all kinds of 3rd party libraries also use it and you end up with lot's of events flowing from $rootScope through the entire scope tree. I have bad feelings about that.

caitp commented 10 years ago

Currently, every $scope event 'bubbles' upwards or downwards unless stopPropagation is called...

I was thinking that it would be useful to be able to ask the event not to bubble (via a parameter or a separate function call).

Since the code for $emit and $broadcast is very similar, we could also shrink the code a bit by having a single function performing most of the work, and using closures to customize the behaviour slightly for each. Using this strategy (similar to jQuery's event dispatch mechanism), it could be simple to prevent an event from bubbling, and also simplify maintenance in the future.

This is kind of off topic for this issue, but I think it's probably a good idea, even if it introduces some minor breaking changes

cburgdorf commented 10 years ago

I think the upward bubbling is fine and maybe even the downward bubbling has it's use cases (even if I don't see them) but my main point is that application wide events should not bubble downwards through the entire scope tree.

caitp commented 10 years ago

But who knows what is intended to be an "application-wide" event except for the thing which emits it?

Anyways, I think $broadcast and $emit both return an event object, so stopPropagation() could be called at that point --- I just think it would be nicer/simpler to be able to prevent propagation during dispatch itself. edit I guess $broadcasted events are not cancellable, who knew.

cburgdorf commented 10 years ago

Well things like $locationChangeStart are truly application wide. As I said, I don't see a use case for $broadcasting at all so from my point of view it would be fine to have all events that now $broadcast on $rootScope to be migrated to just be $emitted on $rootScope.

Regarding stopPropagation(), that doesn't solve the problem. 3rd party libraries like ui-router or angular-translate $broadcast events on $rootScope to listen to them internally as well so they create lots of events that bubble through the entire scope tree. What for? Each scenario where $rootScope.$broadcast + $scope.$on is used can be replaced by the much more lightweight $rootScope.$emit + $scope.$onRootScope pattern that doesn't bubble through the entire scope tree but rather uses the $rootScope as a flat EventBus.

wesleycho commented 10 years ago

The $scope event hierarchy is separate from DOM events.

Have any particular examples of third party libraries using $rootScope.$broadcast? I haven't come across that myself except maybe UI Router, but UI Router is built to be a more powerful extension of ngRoute, which also does $rootScope.$broadcast to propagate changes in the DOM due to state change.

Some application wide events on $scope should bubble downwards I think - for example, let's take a look at something internal to $scope, the $destroy event. When a $scope is slated for destruction, Angular sends this event via $scope.$broadcast('$destroy'). This makes it a good use case with controlling what pieces of the DOM you want active via $scope hierarchy, and actions you want to fire based on that in a controlled fashion. I would imagine it being the driving philosophy behind the broadcasting of events like $routeChangeStart and $routeChangeSuccess in ngRoute for example.

A custom event bus could be nice, but you run into likely having to rewrite a lot of code. Maybe an amenable proposition is to have a method called $rootScope.$instantBroadcast that propagates it to all listeners simultaneously, with perhaps no guarantee of order.

cburgdorf commented 10 years ago

Yep, ui-router and angular-translate for instance use it.

You are right, the $destroy event is a great use case where a $broadcast makes sense as you really only want it to be pushed to a limited group of receivers. But for things like $routeChangeStart I think you want to have it application wide and therefore it should be fine to have it as a flat event $emitted on $rootScope.

And further more, I noticed quite a lot people rolling their own event systems to be used with Angular. That clearly shows something is wrong I think.

If the core team decides that a dedicated event bus should be used for event communication in an angular app, then that's fine with me but it should be something the framework provides. I personally don't think that it has to be a dedicated event bus as I see $rootScope in that role. I now use this $rootScope.$emit+ $scope.$onRootScope pattern and are happy to have flat events that don't drain performance while keeping the auto resource cleaning semantics.

caitp commented 10 years ago

Does $onRootScope do something that $rootScope.$on wouldn't do?

Anyways, I still sort of think the ideal solution would shrink code (by moving the common event dispatch code into its own routine) -- Like dispatcher(name, argsArray, propagationFn) --- Then $emit would call dispatcher(name, [args], dispatchDownFn), $broadcast would call dispatcher(name, [args], dispatchUpFn), and a new function $dispatch (for example) would target a single scope without bubbling, and would simply call dispatcher(name, [args], false).

I think this is probably the cleanest approach with the current codebase --- It would simplify things, shrink code, and also provide a nice, simple means of targetting a single scope without bubbling. I think that's pretty cool.

I'll work on a patch for this this morning just as an experiment --- But I don't really see $onRootScope as providing a helpful solution, because it's not clear what it's actually supposed to do which isn't handled by the current infrastructure already.

cburgdorf commented 10 years ago

@caitp indeed it does :) It automatically unsubscribes from the event when your local $scope gets destroyed. If you just bind with $rootScope.$on from within a controller than you need to take care about the unbinding yourself when your local $scope gets destroyed. Otherwise you end up with event handlers summing up and creating lots of memory leaks.

If you use $scope.$onRootScope then you subscribe to the event that was $emitted on $rootScope but you also unsubscribe when your local $scope gets destroyed.

I just paste this in here again:

angular
    .module('MyApp')
    .config(['$provide', function($provide){
        $provide.decorator('$rootScope', ['$delegate', function($delegate){

            $delegate.constructor.prototype.$onRootScope = function(name, listener){
                var unsubscribe = $delegate.$on(name, listener);
                this.$on('$destroy', unsubscribe);
            };

            return $delegate;
        }]);
    }]);
cburgdorf commented 10 years ago

Since @wesleycho came up with a good use case for $broadcast I came to a good definition on when to use $broadcast and when to use $emit.

Use $broadcast only if not on $rootScope because then you mean to only notify a subset of receivers! That's the case for $scope.$broadcast('$destroy'). If however, you are about to write $rootScope.$broadcast, STOP! So you want to notify everyone? Then just declare it as an application wide event and use $rootScope.$emit.

Let's use a metaphor. Say you are the government and you want to notify some specific people that they have to pay additional taxes, then you write them a letter. If however, you want to raise taxes for everyone, then you just put it on the media to let everyone know without explicitly writing a letter to everyone since it's cheaper.

You can extend the metaphor and say that if you are a very rich country (desktop ;-)) then you might just don't care enough and send a letter to everyone but if you are a poor country (mobile) then you definitely want to avoid sending letters to everyone if you want to inform everyone about it.

Does that make sense to you?

IgorMinar commented 10 years ago

I like this proposal. I think that we should look at it in 1.3

cburgdorf commented 10 years ago

Great to hear that you like it! I'll prepare a PR soon.

0x-r4bbit commented 10 years ago

:+1:

IlanFrumer commented 10 years ago

+1

psagar commented 10 years ago

+1

kbudde commented 10 years ago

+1

imanderson commented 10 years ago

+1 !

joeRinehart commented 10 years ago

+1.

I come from a Flash/Flex background where two of our key frameworks - Swiz and RobotLegs - all used a central event dispatcher. It was key in building very large, flexible/modular/all that stuff apps.

marcalj commented 10 years ago

+1

harmon commented 10 years ago

I've incorporated your stack overflow snippet (http://stackoverflow.com/a/19498009/288703) into my app. Thanks!

Guuz commented 10 years ago

I have no "problems" with the current use of $emit and $broadcast, but I see how it can be more efficient when it's a "flat event bus". In a lot of the scenarios I have come across, a "flat event bus" is enough and I don't need bubbling. But if you're not aware of the lesser performance this is easily overlooked. Therefore keeping it as is (simple) is nice for the Angular beginners. A more efficient method for advanced users (where the performance difference can become noticeable) should be optional, but provided.

tl;dr :+1:

Maidomax commented 10 years ago

:+1:

SimplGy commented 10 years ago

:+1: for a flat event bus that auto unbinds.

SimplGy commented 10 years ago

Use cases for a shared event bus might help :)

fernandezpablo85 commented 10 years ago

Hi, why hasn't this been merged yet?

rodrigoreis22 commented 10 years ago

+1

WhatFreshHellIsThis commented 10 years ago

+1, I guess we'll have to implement manually for now: https://gist.github.com/turtlemonvh/10686980

maximvl commented 10 years ago

@WhatFreshHellIsThis are you sure this works? I'm registering $on listeners before emitting events, but they do not receive them.

WhatFreshHellIsThis commented 10 years ago

@maximvl Yup, definitely, using it now all over the place. Not sure if the code is identical, I've done a lot since then. Not an Angular service / directive expert so I'm just going to dump everything relevant here that is working for your reference.

Here are some working snippets taken directly out of a project:

Here is the service from it's own file: ay-msg.js:


angular.module('XXXApp')

.factory('ayMsg', ['$rootScope', function($rootScope) {
    var ayMsg = {};
    ayMsg.emitMsg = function(msg, data) {
        data = data || {};
        $rootScope.$emit(msg, data);
    };
    ayMsg.onMsg = function(msg, func, scope) {
        var unbind = $rootScope.$on(msg, func);
        if (scope) {
            scope.$on('$destroy', unbind);
        }
    };
    return ayMsg;
}]);

Referenced in the start page: index.html:

    <script src="common/services/ay-msg.js"></script>

And of course you do not need to declare it in your common declarations, at least I don't: app.js:

angular.module('XXXApp', ['ngSanitize',
    'ngRoute',
    'ngResource',
    'ngSwitchToggle',
    'ui.select',
    'btford.socket-io',
    'LocalStorageModule',
    'datePicker',
    'angularFileUpload',
    'decipher.tags',
    'ui.bootstrap',
    'headroom'

]) 
.config(
...etc basically you can see I didn't declare it here...

Controller that receives messages snippet: nav.js:

angular.module('XXXApp')
    .controller('NavCtrl', ['$scope', 'ayMsg', '$location',
        function($scope, ayMsg, $location) {
...etc ....

Later on....
 ayMsg.onMsg('ayCanAdd', function(evt, data) {
                $scope.ayCanAdd = data.active;
            }, $scope);

And the sender controller... user-edit.js:

angular.module('XXXApp')
    .controller('UserEditCtrl', ['ayMsg', '$scope', '$routeParams', 'ayApi', '$location',
        'aySessionMgr', 'utilService', '$upload', '$http', '$modal',
        function(ayMsg, $scope, $routeParams, ayApi, $location, aySessionMgr, util, $upload, $http, $modal) {

...etc etc much unrelated  code here until...

//Sender ...
 ayMsg.emitMsg('ayCanAdd', {
                active: true
            });

[update: just thought of something that might be giving you grief] Remember that the controller that receives must be active as well as the sender, i.e. in my case I use the nav.js controller constantly throughout the lifetime of the app as it handles the navigation area updates depending on context.

You can't send a message to a controller that's not instantiated, i.e. from one view to another view that is not currently active, that might be your problem?

Hope this helps. Cheers!

maximvl commented 10 years ago

@WhatFreshHellIsThis thanks for detailed answer! I actually found that something like angular.injector(['ng', 'myApp']).get('msgBus').emitMsg('test', 'test message'); changes the $rootScope (its id changes too) and removes all $$listeners, and other properties as well. emit, of course, does not work. Is this correct behaviour, because official guide says Every application has a single root scope? (tested on angular 1.3.0 beta 10)

WhatFreshHellIsThis commented 10 years ago

Sorry @maximvl I don't have the experience to help you with your follow up question. I'm using v1.2.16 at the moment and it's definitely working if that makes any difference.

drtobal commented 10 years ago

+1

maximvl commented 10 years ago

Just tried 1.2.16, its $rootScope is changing too. Can anyone explain this?

maximvl commented 10 years ago

Here is a quick fiddle of how root changes and listerens disappear: http://jsfiddle.net/L7Hm5/1/

Scoup commented 10 years ago

@maximvl you should use $injector instead angular.injector, like this: http://jsfiddle.net/L7Hm5/2/

source: http://stackoverflow.com/questions/12758157/angularjs-how-to-inject-dynamically-dependence-in-a-controller

petebacondarwin commented 10 years ago

Moving the discussion to #5507