vuejs / vue

This is the repo for Vue 2. For Vue 3, go to https://github.com/vuejs/core
http://v2.vuejs.org
MIT License
208.03k stars 33.69k forks source link

Stop propagation by default in `$broadcast()` and `$dispatch()` #1175

Closed yyx990803 closed 9 years ago

yyx990803 commented 9 years ago

Problem

The event system seemed like a useful idea when I first implemented it. It is still very useful internally, but I've found that I almost never use it in my application code because the accidental complexity $broadcast() and $dispatch() can introduce. Here are some issues:

  1. Their behavior relies on the structure of the component tree. On the surface it decouples the components, but in fact it still couples them quite tightly. Once a event relationship is set up, your components need to stick to that structure; you can accidentally break your event chain by moving the components around.
  2. Namespacing. When you listen to an event, you have no idea if a child component somewhere might dispatch an event with that name and screw things up. You just can't prevent that.
  3. Events are often (ab)used to trigger state changes, but their propagation inside the component tree is very abstract and implicit. Because the events can go both ways, the events flow inside a large application quickly gets very hard to wrap your head around, and it becomes even harder to understand where your app state to change originated from. A lot of times, a simple global event bus can make things centralized and thus a lot easier to understand. (You can even adopt Flux if you want to go further)

    Proposal

  4. Deprecate $broadcast() and $dispatch(). Other event methods still remain, which means Vue isntances are still event emitters. Stop propagation by default in $broadcast() and $dispatch(), unless the user calls the second argument propagate.
  5. Expose a global utility so users can construct their own event bus. Something like new Vue.Emitter().
  6. (Outside of core) Investigate recommendation for state management. You can use libraries like vuex or Redux.
Mat-Moo commented 9 years ago

I'm sure you've read all my code and are trying to break everything I've written :) I currently use a dispatch event on a component to tell it's parent that it's now selected (Which is bound to the main app), this then broadcasts to all the other components a message to say that a new component is selected. Works exactly as I thought it should and made sense. I could change this to use a 2 way prop, but when you start going through multiple layers it gets a bit messy IMO.

If your still using them internally then why not let them live and add a new global emitter that people can use if the current system doesn't work for them?

dominiquedutra commented 9 years ago

I use both heavily, but am more then willing to rewrite everything as its really starting to feel messy.

azamat-sharapov commented 9 years ago

but when you start going through multiple layers it gets a bit messy IMO

That's why I wrote about something like $context earlier, but this probably won't come soon :)

As for events, I use them sometimes and global utility would be useful. For me, there were some interesting cases for events while dev, it's not about only centralizing app data and state.

mark-hahn commented 9 years ago

Adding global would be nice but please don't remove the current ones. I find it useful to know the intent. It improves readability. I use all three heavily.

On Mon, Aug 17, 2015 at 11:06 AM, Azamat notifications@github.com wrote:

but when you start going through multiple layers it gets a bit messy IMO

That's why I wrote about something like $context earlier, but this probably won't come soon :)

As for events, I use them sometimes and global utility would be useful. For me, there were some interesting cases for events while dev, it's not about only centralizing app data and state.

— Reply to this email directly or view it on GitHub https://github.com/yyx990803/vue/issues/1175#issuecomment-131911704.

nirazul commented 9 years ago

I also heavily rely (relied) on the Event Bus in some (now finished) projects. It really felt a bit messy and eventually what I did 90% of the time, was sending events to a controller instance at the top, where they were handled and sometimes another event was broadcast down the chain. So I sort of globalized a localized system.

If you implement something like Vue.Emitter (I really love the Idea), please make sure that it can be switched out with custom modules from the node ecosystem like wolfy87.

One thing to consider. Most of us will use some sort of modularisation like webpack or browserify. Does it really make sense to implement an EventBus when you can have an instance of an existing EventBus on your fingertip by writing require("myFavouriteEventEmitter")?

yyx990803 commented 9 years ago

@mark-hahn 1.0.0-alpha will not remove anything, you have plenty of time to migrate. I honestly think the benefits of decoupling the event system from the view structure outweighs the express of intent of $broadcast and $dispatch. For example, with a global event bus, when you grep for .emit('event') and .on('event') you know the former will trigger the latter, but when the event propagation depends on how your components are structured, you can't be so sure.

@Nirazul currently, all Vue instances implement the emitter interface already. So it's just a matter of exposing some internals that's already there. It's totally optional, you sure can use whatever external event library you want. Since without $broadcast and $dispatch a global emitter would be a common pattern, it's good to make sure users not using certain build tools still have access to the same functionality.

mark-hahn commented 9 years ago

I honestly think the benefits of decoupling the event system from the view structure outweighs the express of intent of $broadcast and $dispatch.

I just realized that I was arguing strongly about simplicity and removing things so now I agree these can be removed.

On Mon, Aug 17, 2015 at 12:49 PM, Evan You notifications@github.com wrote:

@mark-hahn https://github.com/mark-hahn 1.0.0-alpha will not remove anything, you have plenty of time to migrate. I honestly think the benefits of decoupling the event system from the view structure outweighs the express of intent of $broadcast and $dispatch. For example, with a global event bus, when you grep for .emit('event') and .on('event') you know the former will trigger the latter, but when the event propagation depends on how your components are structured, you can't be so sure.

@Nirazul https://github.com/Nirazul currently, all Vue instances implement the emitter interface already. So it's just a matter of exposing some internals that's already there. It's totally optional, you sure can use whatever external event library you want. Since without $broadcast and $dispatch a global emitter would be a common pattern, it's good to make sure users not using certain build tools still have access to the same functionality.

— Reply to this email directly or view it on GitHub https://github.com/yyx990803/vue/issues/1175#issuecomment-131941355.

TerenceZ commented 9 years ago

For building an application, the global event bus pattern will lead the app to more easier to manage, I think. But for building common UI components, I think the $broadcast and $dispatch are useful to communicate between some relative components like menu, menu-item and submenu because the structure are intend to be coupled and the structure information should be also carried by the events (implicit by the $broadcast and $dispatch). For example, when the menu closes, it should also let all submenus close too, it is easy to do this by $broadcast an menuclose event.

mark-hahn commented 9 years ago

when the menu closes, it should also let all submenus close too, it is easy to do this by $broadcast an menuclose event.

And this event can have only one name without namespace problems, where a global bus would require some awkward namespacing code.

On Tue, Aug 18, 2015 at 10:32 PM, terencez notifications@github.com wrote:

For building an application, the global event bus pattern will lead the app to more easier to manage, I think. But for building common UI components, I think the $broadcast and $dispatch are useful to communicate between some relative components like menu, menu-item and submenu because the structure are intend to be coupled and the structure information should be also carried by the events (implicit by the $broadcast and $dispatch). For example, when the menu closes, it should also let all submenus close too, it is easy to do this by $broadcast an menuclose event.

— Reply to this email directly or view it on GitHub https://github.com/yyx990803/vue/issues/1175#issuecomment-132451404.

yyx990803 commented 9 years ago

@TerenceZ @mark-hahn I can see the use case here, but how do we control of boundary of these events? Right now to prevent the event dispatched by a menu-item to go outside menu, we'd have to remember to return false in menu's corresponding event handler. Same for $boardcast if menu-item can contain inserted content. If we don't do this we have the same implicit namespace problem. But having to remember to manually do this is a fragile pattern. This is the part that I am most unhappy about with the current implementation, and I'd love to see if there's a way to solve this problem while keeping the usefulness.

mark-hahn commented 9 years ago

how do we control of boundary of these events?

I'm confused. A global bus has no boundaries at all, right? I must be misunderstanding your meaning of boundary.

On Wed, Aug 19, 2015 at 10:56 AM, Evan You notifications@github.com wrote:

@TerenceZ https://github.com/TerenceZ @mark-hahn https://github.com/mark-hahn I can see the use case here, but how do we control of boundary of these events? Right now to prevent the event dispatched by a menu-item to go outside menu, we'd have to remember to return false in menu's corresponding event handler. Same for $boardcast if menu-item can contain inserted content. If we don't do this we have the same implicit namespace problem, but having to remember to manually do this is a fragile pattern. This is the part that I am most unhappy about with the current implementation, and I'd love to see if there's a way to solve this problem while keeping the usefulness.

— Reply to this email directly or view it on GitHub https://github.com/yyx990803/vue/issues/1175#issuecomment-132721491.

yyx990803 commented 9 years ago

@mark-hahn yeah, that's true. I do agree that the global event bus has its own issues. Maybe what I want is for event listeners to return false by default - i.e. when a $broadcasted / $dispatched event is handled, it just stops, unless the handler explicitly pass it on with return true. This would naturally contain the events sent out within a subtree so that they don't accidentally affect other parts of the application.

mark-hahn commented 9 years ago

I would prefer to keep it the way it is, but that's clearly just a personal preference (and I'm lazy). Arguments can be made for either default return. Good coding would make every return explicit. (grin)

On Wed, Aug 19, 2015 at 12:08 PM, Evan You notifications@github.com wrote:

@mark-hahn https://github.com/mark-hahn yeah, that's true. I do agree that the global event bus has its own issues. Maybe what I want is for event listeners to return false by default - i.e. when a $broadcasted / $dispatched event is handled, it just stops, unless the handler explicitly pass it on with return true.

— Reply to this email directly or view it on GitHub https://github.com/yyx990803/vue/issues/1175#issuecomment-132745809.

young-steveo commented 9 years ago

@yyx990803 I like the idea of defaulting off and specifying that an event should propagate further, but return true;—despite it's simplicity—it's not very semantic.

What about providing the handler a callback that should be executed if they want the event to propagate? Something like this:


// this handler will not propagate
this.$on('eventA', function(data, propagate) {
   // ... do something with data
});

// this handler will propagate
this.$on('eventB', function(data, propagate) {
   // ... do something with data
  propagate();
});
yyx990803 commented 9 years ago

@young-steveo agreed, that's definitely worth considering.

mark-hahn commented 9 years ago

return true;—despite it's simplicity—it's not very semantic.

It's a standard used everywhere. If everyone know what it means then it is semantic.

while providing an easy migration path.

It may seem easy to you but to me it looks like a lot of unneeded complexity to me. Why does everyone want to change something that works? Can someone show me an example where the current methods don't work or they break something?

On Wed, Aug 19, 2015 at 2:23 PM, Ryan P Kilby notifications@github.com wrote:

import Vue from 'vue';

Vue.extend({ emitters: { dispatch: Vue.DispatchBus, }, components: {...}, created: function() { this.$on(dispatch, function(data, bus, propagate) {...} } methods: { theThing: function() { } } });

A couple of thoughts on deprecating the existing emitters, while providing an easy migration path.

  • Different buses provide different behaviors of how events are propagated throughout the vue hierarchy.

Just a thought: the current $dispatch emitter is similar to dom event bubbling. Whether or not that's a good thing, I don't know, but it's a familiar mental model to regular DOM event handlers.

Maybe a solution is to separate each emitter behavior into it's own bus, and vues would register multiple buses. Some possiblities:

  • dispatch bus: vue hierarchy aware, only sends events up the vue hierarchy.
  • broadcast bus: vue hierarchy aware, only sends events down the vue hierarchy.
  • global bus: vue hierarchy agnostic, broadcasts events to all vues in its registry.
  • local bus: like the current $emit. It could maybe be a default or required bus?

Other thoughts:

  • $emit could take an optional bus name to specify which bus to emit the message onto (defaults to the local bus otherwise).
  • Existing $dispatch and $broadcast methods would be provided by their respective buses as shortcuts to calling $emit('').
  • Child vues/components automatically register

— Reply to this email directly or view it on GitHub https://github.com/yyx990803/vue/issues/1175#issuecomment-132790055.

rpkilby commented 9 years ago

Sorry - prematurely submitted the comment while hashing out my thoughts. Thought I was able to delete it before it sent out an email.

yyx990803 commented 9 years ago

@mark-hahn imagine you have a component, say menu, which includes a private child component, menu-item, and the menu-item communicates to menu by dispatching an event, e.g. closed.

Now if you want to make this component reusable, you don't want the closed events dispatched from the child to leak outside of menu, and potentially affect the higher-up component that also happens to listen for a closed event. Sure you can avoid this by namespacing all the events, but that's when you have full control over the whole application. If the component is used by someone else in an unknown environment, you have no guarantee that the dispatched event won't accidentally trigger some unwanted handler up the tree.

To ensure the event doesn't leak, you have to "intercept" it at the menu level, and with the current API you have to explicitly return false, which probably very few people do.

mark-hahn commented 9 years ago

That is a good example. I understand now.

Then I'll argue to reverse the return true/false default. I will continue to argue against anything more complex.

On Wed, Aug 19, 2015 at 3:30 PM, Evan You notifications@github.com wrote:

@mark-hahn https://github.com/mark-hahn imagine you have a component, say menu, which includes a private child component, menu-item, and the menu-item communicates to menu by dispatching an event, e.g. closed.

Now if you want to make this component reusable, you don't want the closed events dispatched from the child to leak outside of menu, and potentially affect the higher-up component that also happens to listen for a closed event. Sure you can avoid this by namespacing all the events, but that's when you have full control over the whole application. If the component is used by someone else in an unknown environment, you have no guarantee that the dispatched event won't accidentally trigger some unwanted handler up the tree.

To ensure the event doesn't leak, you have to "intercept" it at the menu level, and with the current API you have to explicitly return false, which probably very few people do.

— Reply to this email directly or view it on GitHub https://github.com/yyx990803/vue/issues/1175#issuecomment-132808272.

young-steveo commented 9 years ago

@mark-hahn In my opinion, literal booleans can easily become anti-patterns when used as function parameters or return values (unless the function name semantically indicates that you are expecting a boolean, like this.isReady() or something like that).

Here's an example where boolean parameters make the code less semantic:

this.save({ id : 5, value : 'foo' }, false, true);

In that example, it looks like the function is saving an object... but what is the third param true? What is false doing in the second param? You would have to look up the function definition to find out what those params are doing. A better API would be something like:

this.save({ id : 5, value : 'foo' }, { log : false, upsert : true });

Now you can pretty much reason out that you are saving an object, and you want the upsert option to be true, and you don't want to log anything. This is more semantic.

Now, regarding return true;, we can make the same analogies. Returning true does not indicate what true really means.

function handleEvent() {
  // do stuff with backend, then...
  return true;
}

What does return true mean here? Does it mean that an event was successfully handled? Does it mean that the event should propagate? Does it mean that the backend operation was successful?

If everyone know what it means then it is semantic.

That's not really true (pun intended? j/k). If everyone knows what it means, then it is expected. It is intrinsic. But it is not semantic, or explicit.

mark-hahn commented 9 years ago

I agree with everything you've pointed out, although using return false is less ambiguous than your call example this.save({ id : 5, value : 'foo' }, false, true);.

I've always seen return false to mean to stop propagating but here we are talking about reversing it, which now that I think about it would be really bad because it would be the opposite of the standard. That would really confuse everyone. So maybe having a simple stopPropagation() or continuePropagation() or passEvent() call would be unambiguous and semantic. I still argue that return false meaning to stop propagation would be fine.

I know my opinion is flip-flopping all over the place. I will try to think things out better before typing. At least I'm stimulating thinking (I hope).

BTW, googling "semantic" results in "relating to meaning in language or logic." The meaning of return false in events is clearly to stop propagating, so how is that not semantic? this.zxy(true, false) is not semantic.

young-steveo commented 9 years ago

@mark-hahn I completely agree that using return false in an event handler to stop propagation is very common. However, I firmly reject that it is semantic.

The meaning of return false in events is clearly to stop propagating, so how is that not semantic?

Knowing a thing does not make it explicit. You subjectively know the meaning of return false because you are applying context and experience. But if you look at return false objectively, it's just a boolean. It has no further context beyond true or false. I would argue that return "stop propagating" would be a much more semantic—albiet ugly and brittle—pattern.

All that said, I guess return false gets the job done at the end of the day.

dominiquedutra commented 9 years ago

When I first started working with components in Vue I assumed the event propagation stopped after the first component listening to it got hit. Thats what made sense to me - of course after carefully reading the docs I was aware of the return false;

If I want to propagate it further i'll just do something like propagate().

VladimirShestakov commented 9 years ago

It is necessary to make $on("*") for listening all events from $dispatch (or check that the event is dispatched in the handler). If event with attribute "all" (optional) in the root component then call $broadcast(...)

Stop propagation by default :+1:

davidkhess commented 9 years ago

Couple of thoughts:

  1. You could fix the namespace issue by using reverse domain naming. For instance, "com.example.closed". Kind of verbose, but it would work.
  2. What if a ViewModel could declare that it terminates the event hierarchy above it (as a leaf node) and starts a new event hierarchy from that point down. Events don't propagate across it. $broadcast sends an event down on the new hierarchy and $dispatch sends an event up the "parent" event hierarchy.

While the "propagate" call approach would work, it has to be used consistently in any $on handler. With 2. it would be just a declarative setting on a ViewModel and $on handlers wouldn't need to worry about it.

Option 2. could also be generalized into the event bus pattern (call it Option 3). The root ViewModel has a single event bus inherited by all children in the hierarchy until hitting a ViewModel that declares that it and everything below it should share a new bus.

I actually like Option 3.

yyx990803 commented 9 years ago

@davidkhess interesting ideas, but there's one problem with this what I call "event root" solution: it only defines the upper-bound of the events, but for lower-bound it relies on a child component to also declare itself as event root, which is not a strong guarantee.

davidkhess commented 9 years ago

True enough – i.e. an opt-in mechanism for event forwarding so that the default behavior is safer. I can't think of a way to do that in Option 3 declaratively without creating a lot of extra declarative setup. At least propagate is isolated to the event handlers themselves.

+1 for propagate

yyx990803 commented 9 years ago

Ok, so the problem with the extra propagate argument is that events may emit with unknown length of arguments:

this.$emit('a', 1, 2, 3)
this.$emit('a', 2, 3)

If we simply append propagate to the end of the argument list, the handler don't really know which one it is, because the arguments length can be different.

I guess we'll have to go with return true here - although less explicit, it seems a straightforward solution.

yyx990803 commented 9 years ago

Implemented in alpha branch.