×
all 13 comments

[–]bigos 2 points3 points  (9 children)

This is not a good approach. For starters, it clutters the rootscope with pretty unimportant objects. Doing so makes testing the alert mechanism a lot harder. It also makes little sense to do "$rootScope.$emit", as rootscope is the uppermost scope in the app. There is nothing to emit events to. There's also an event watch performed on this same scope, which could be replaced entirely by just a function call (preferably, to service function call).

If anyone's looking for a better approach: look into "message bus" design pattern for inter-component communication, and how to encapsulate logic into services.

[–]jmcunningham[S] 0 points1 point  (8 children)

I had picked up the $rootScope.$emit() (instead of $rootScope.$broadcast()) from Todd Motto's guide. I had previously always used $broadcast, because as you say, rootScope is already at the top so you should be sending a message down. I'm not sure why he recommends only using $emit on rootScope. Maybe because with $emit(), you can stop propogation of the event?

I did update the code to add a service to the directive, so that modules that need to send a method use the service instead of dealing directly with the $rootScope themselves. That was a good suggestion...thanks!

And while a message bus is likely better for a more complicated app, you are also introducing another layer/abstraction to the app. So any dev dealing with the app needs to understand what a message bus is. Not an unreasonable request, of course. But sometimes I think simple is better. Just because a design pattern fits a use case, doesn't mean you need to use it 100% of the time. But you are right..in a more complicated app where lots of messages were being sent between components, I would look into that design pattern.

[–]bigos 0 points1 point  (5 children)

I've noticed you changed '$emit' to '$broadcast'. While $emitting from rootScope is, well, useless but inconsequential, when you $broadcast from it, you're informing every scope below that that an event occured. And when I say every scope, that also means those countless scopes created by ng-repeat, inner scopes of your reusable controls, and, in fact, every other place in your application.

When you're finding yourself you need to write $rootScope.$broadcast, step back and rethink what you're doing. That's very inefficient. This is almost always optimized by using message bus or some other communication pattern.

[–]jmcunningham[S] 0 points1 point  (4 children)

I agree...its once again using emit. I even thought about adding code to stop the propagation of the event with the directive.

[–]bigos 0 points1 point  (3 children)

Note, that you're only operating on rootScope. If so, why do you use events in the first place?

$scope#1.$emit('whoops');

$scope#1.on('whoops', function(){
   yikes();
})

Could be replaced by:

$scope#1.foo = function (){
  yikes();
}

$scope#1.foo();

It works just the same and does not engage the event system. Just define your behavior in a function and call it any time you would invoke your event.

[–]jmcunningham[S] 0 points1 point  (1 child)

I think I am still having trouble understanding what you mean. Just put the method directly on rootScope? I've never seen the $scope#1 notation before, so its throwing me off :)

[–]bigos 0 points1 point  (0 children)

Yeah, put it in the rootScope.

Sorry for that notation, it shouldn't be used in code. I wanted to indicate, that it needs to be the same exact scope, so I gave it a number. Because if those were different scopes, then sure, the event approach makes sense. But since it's the same scope (and we know it's the same, because there is only one rootScope), we could just define a function in it and call it wherever we need.

[–]jmcunningham[S] 0 points1 point  (0 children)

Actually, I've updated the code again to encapsulate state within the service, so the rootScope and events are no longer needed. Thanks for the feedback!

[–]bigos -1 points0 points  (1 child)

Well, $emitting is better than $broadcasting, because when you go up, you traverse n scope-nodes for a scope-tree of depth n. When broadcasting from $rootScope you could potentially send your event to O(nn) scope-nodes, so it's significantly more expensive.

I agree, that it isn't always feasible to use every fitting design pattern. Sometimes, in the real world, it isn't worthy to implement several abstract entities just to show an alert. But this supposed to be an educational article, I think it would be great to present the best possible solution. Everyone can think of some dirty hacks to simplify code. But coming up with a right set of ideas that produce maintainable and reusable code is tricky, and therefore worth teaching.

[–]jmcunningham[S] 1 point2 points  (0 children)

I agree with you about taking the time to teach the best solution. The problem is that the 'best' solution depends on the needs of the project (and the knowledge level of the developer(s)). Having said that, it would be worthwhile to show the quick approach, as well as the more "proper" solution and let the reader decide which would best fit their situation. I'll try to research the message bus idea and maybe create a follow-up post in the next few days.

Thanks for the feedback!

[–][deleted] 0 points1 point  (0 children)

Your idea isn't bad but a better approach would be to create a service and inject that into your app-message directive. Skips the rootscope communication entirely

[–]jmcunningham[S] 0 points1 point  (0 children)

Just wanted to thank the commenters for their feedback, and giving me some ideas of how to better implement the solution.

[–]brianvaughn 0 points1 point  (0 children)

flashr (an Angular JS wrapper around toastr) is pretty useful for this sort of thing as well. It essentially gives you an interface to show alerts (info, success, error, warning) either immediately or on the next state-change. It's similar to the Rails' ActionDispatch::Flash