Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bubble events on fire #1117

Merged
merged 17 commits into from
Aug 31, 2014
Merged

Bubble events on fire #1117

merged 17 commits into from
Aug 31, 2014

Conversation

martypdx
Copy link
Contributor

Start of event bubbling very simple implementation. Component events propagate under component.eventName. Test sums it up:

Component = Ractive.extend({
    template: '<span id="test" on-click="someEvent">click me</span>'
});

Middle = Ractive.extend({
    template: '<component/>'
});

ractive = new Ractive({
    el: fixture,
    template: '<middle/>',
    components: {
        component: Component,
        middle: Middle
    }
});

ractive.on( 'component.someEvent', function ( event ) {
    t.ok( true );
    t.equal( event.original.type, 'click' );
});

simulant.fire( ractive.findComponent('component').nodes.test, 'click' );

There's a second commented out test for events declared by parent on component. Need some more thought on what should happen with these.

Also no consideration for additional context beyond original event.

@fskreuz
Copy link
Contributor

fskreuz commented Aug 15, 2014

Awesome, just what I was looking for. That takes care of the hundred lines "bridging events" code. 👍

@fskreuz
Copy link
Contributor

fskreuz commented Aug 15, 2014

Is this related to #1099 ?

@martypdx
Copy link
Contributor Author

@fskreuz yes, though newer PR from @evs-chris is #1116. it is using a * or ** on original event to indicate how far to bubble up.

@fskreuz
Copy link
Contributor

fskreuz commented Aug 15, 2014

@martypdx I think bubbling should be similar to the DOM's bubbling, which bubbles all the way up unless something prevents it in the middle, like event.stopPropagation().

I imagine it looking like this on the DOM.

<body>
  <middle>
    <component on-click="someEvent"> 
  </middle>
</body>

And the JS for it

// Component that fires event
Component = Ractive.extend({
  template: '<span id="test" on-click="someEvent">click me</span>'
});

// Middle component that uses component
Middle = Ractive.extend({
  template: '<component />',
  components: {
    component: Component
  },
  init: function() {
    this.on({
      someEvent: function() {
        // someEvent will not bubble anymore
        event.stopPropagation();
      }
    });
  }
});

ractive = new Ractive({
  el: fixture,
  template: '<middle/>',
  components: {
    middle: Middle
  }
});

// someEvent, regardless of who fired it (no namespace)
ractive.on('someEvent', function(event) {
  // normally should fire
  // but will not because of a stopPropagation() in Middle
});

@martypdx
Copy link
Contributor Author

@fskreuz @evs-chris One of the current issues is that the event parameter is not present for events fired directly via ractive.fire('someEvent', arg1, arg2, argN).

It will be a BREAKING CHANGE, but I think would be good idea to add it. It would make things consistent with event plugins and we could construct the context for the ractive instance.

I can never find existing gh issues like @Rich-Harris, but there was also some talk of moving the e event arg to the end of the arguments so that we could support method calls from events.

Or maybe event args should be passed as properties of e, again that's how it works in custom event plugins. not really true, args get added

@fskreuz
Copy link
Contributor

fskreuz commented Aug 15, 2014

Just some wild suggestions based on other libraries. An event object should be present anywhere to carry event details like who triggered, event name, targets, delegated targets etc. (like jQuery). Data transport could vary (args vs data object).

this.on('event',function(eventObj, arg1, arg2, arg3){
  //eventObj houses all event data
});

this.fire('eventName',arg1,arg2,arg3...);

or

this.on('event',function(event){
  //event object puts the passed data into a data property
  //event.data.foo;
  //event.data.bar
});

this.fire('eventName',{
  foo : 'hello',
  bar : 'world'
});

@martypdx
Copy link
Contributor Author

@fskreuz yes, I'm coming to the same conclusion. We just need to add the event object for ractive.fire calls.

@martypdx
Copy link
Contributor Author

I added in '*' notation idea from @evs-chris, though with more limited meaning of indicated whether or not event should bubble outside of component.

<span id="test" on-click="*someEvent">click me</span>

It seems like right thing for internally in the component to decide whether event is private or should be bubbled. I like * character, but open to any other suggestions.

Initially, I implemented on-click="*.someEvent" to indicate namespace should be added, but that felt weird for internals of a component to know about how it should be propagated. So instead I added a convention to component declaration in template:

<component on-*="foo"/>

This will cause bubbled events to be foo.someEvent. Either on-*='*' or on-*='' will default to component name, so <component on-*="*"/> will bubble up component.someEvent.

Still need to add event object for ractive.fire and I believe component declared events can now be more expressive.

@fskreuz
Copy link
Contributor

fskreuz commented Aug 17, 2014

@martypdx How about a "bubble : true || false" in the component's configuration. By default, components bubble up, like DOM elements. Setting it to false will prevent bubbling. One can still tap into the event via your usual on-someevent="someHandler".

As for namespace some ideas:

A special namespace attribute

how about placing a special namespace attribute to the component, like:

<timewidget ractive-eventns="some.event.namespace" on-someevent="someHandler" />

So the component only knows that it needs to fire someevent with some data. The parent component will be in control of where that event is forwarded. In this case, someevent becomes some.event.namespace.someevent in the outside.

When listening, namespaces can also be wildcarded, similar to observers (yey consistency!):

  • widgets.* - listen for all events in the widget namespace.
  • widgets.*.time - listen for events one level down, for time events.
this.on({
  'widgets.*.time' : function(){
    ...
  }
});

The ractive-* prefix is to make that attribute special, like not to be included in data or whatever.

Define namespace in parent implementation

I personally hate the idea of adding too much doodads in the HTML (hence abandoned Angular for Ractive). So the on-*="" or even my suggestion of ractive-* idea is kinda funky. Also, haven't read the web components spec nor tried polymer, if they have namespacing (will do research).

So to avoid template clutter, how about delegating it to the parent implementation? Something like:

namespace : {
  'some.namespaceName1' : ['evtName1','evtName2',...],
  'some.namespaceName2' : ['evtName3','evtName4',...],
  ...
}

while the component will remain normal:

<timewidget on-someevent="evtName1" />

@martypdx
Copy link
Contributor Author

@fskreuz

to bubble or not to bubble?

Defaulting to bubbling with no default namespace just seems to open ended as events would be everywhere and prone to inadvertent clashes. (Side note: performance is not really an issue. Informal measurements show that checking if a parent if subscribed takes .02ms). So I've tried both: 1) default namespacing outside the component, 2) Opt-in event bubbling. Thus far, I prefer #2:

  • Deciding whether an event should bubble is better as an event-level decision, not a component wide decision.
  • It's very easy to opt-in, 1 character, and it becomes clear looking at a template what events are "public" vs "private" to the component (though options remain for exposing non-bubbling events):
<ul>
    {{#items:i}}
    <li><a on-tap='*select'>{{name}}</a>
       <div on-tap='showDetails'>{{address}}, {{city}} {{state}}<div>
    </li>
    {{/}}
</ul>

context

Another key issue is context for the event. Currently the events exposed by *someEvent bubble with their original context (meaning the original event argument). So in the above example, you get the index and the current item without having to explicitly add them as event args (note that the keypath is probably meaningless outside the component).

I've added an event object to ractive.fire events, which fires initially within the component with event.context = this.data, not particularly exciting, but when bubbled it takes on the external context arguments of the immediate parent. So keypath, index, and context are all correctly set vis-a-vis the parent view.

Both approaches could make sense for a component *someEvent case, so maybe the bubble context outside the component should be declarable as either local or component. Again, it seems most logical at the event source itself. Perhaps a slight different notation, say **someEvent or *.someEvent.

namespacing

Agree on many of your points. I do think that putting it on the template component declaration is often the right place, though your spot on that we need to be careful to not obstruct data params. Need to think some more on this one...

@martypdx
Copy link
Contributor Author

One other note, I went with event.stopBubble() though I'm open to suggestions. event.cancelBubble has baggage as a deprecated precursor to stopPropagation that is not a method, but a property: event.cancelBubble = true. Stopping ractive event bubbling and stopping original event propagation and default seem like orthogonal concepts, so I avoided conflating them.

@MartinKolarik
Copy link
Member

I don't like the idea of using

<component on-*="foo"/>

to add a namespace. It seems more like "call foo on any event". It could be changed to

<component ractive-namespace="foo"/>

or something else but the more I think about this, the more I like DOM/jQuery-like way of doing this:

  • All events bubble.
  • Bubbling can be stopped with event.stopBubble().
  • Events can be fired with any number of namespaces (eventName, eventName.ns1.ns2).
  • Listener can (optionally) select namespaces it cares about (eventName, eventName.ns1.ns2).

This doesn't require adding a new syntax and/or config options and it's very flexible.

@martypdx
Copy link
Contributor Author

Thanks for the feedback @MartinKolarik.

All events bubble.

Sounds like all events bubbling has support. Let's give that a try.

I don't like the idea of using
<component on-*="foo"/> to add a namespace. It seems more like "call foo on any event". It could be changed to <component ractive-namespace="foo"/>

What about event-namespace? We've avoided generic ractive- attributes thus far, starts to feel like ng-. :) Rather it was descriptive to exactly what it is. I still think majority of time, will be same as component name and it will get old doing <component event-namespace="component"/> so shortcut would be good. Maybe <component event-namespace="."/>? Otherwise, does it supply a pattern with * being the event name? <component event-namespace="foo.*"/> yields foo.someEvent. Or do we enforce it just being a prefix so that <component event-namespace="whatever.is.here"/> gets prefixed to whatever.is.here.someEvent?

Events can be fired with any number of namespaces (eventName, eventName.ns1.ns2).

@MartinKolarik not sure what you mean here. How/where is this happening? Do you just mean component attribute can be used to do whatever you want?

@MartinKolarik
Copy link
Member

@martypdx

What about event-namespace? We've avoided generic ractive- attributes thus far, starts to feel like ng-. :)

Fair enough :-) If we went with this approach, I'd say it's always a prefix and <component event-namespace="."/> can be used as a shortcut for <component event-namespace="component"/>.

@MartinKolarik not sure what you mean here. How/where is this happening? Do you just mean component attribute can be used to do whatever you want?

I meant that instead of adding a namespace on component level (<component event-namespace="."/>), you do it on event level.

Component = Ractive.extend({
    template: '<span id="test" on-click="someEvent.component">click me</span>'
});

Middle = Ractive.extend({
    template: '<component/>'
});

ractive = new Ractive({
    el: fixture,
    template: '<middle/>',
    components: {
        component: Component,
        middle: Middle
    }
});

ractive.on( 'someEvent.component', function ( event ) {
    t.ok( true );
    t.equal( event.original.type, 'click' );
});

simulant.fire( ractive.findComponent('component').nodes.test, 'click' );

Listener is called if:

  • event is fired without namespaces (ractive.fire( 'eventName' )),
  • listener subscribed without namespaces (ractive.on( 'eventName' )),
  • at least one namespace is present in both lists (ractive.on( 'eventName.ns1.ns2' ), ractive.fire( 'eventName.ns2' ))

@fskreuz
Copy link
Contributor

fskreuz commented Aug 18, 2014

@MartinKolarik

<span id="test" on-click="someEvent.component">click me</span>

Components should have no idea of the outside world and the namespaces it should use. Makes them more portable by only defining events they emit. The parent component handles namespacing. That way, if you borrow components from another project (like we do now in ours), we just slap it in, change the namespace in the parent template, and we're good to go. We need not tinker with the component code.


This following might be better (I think):

In the component, we just fire of events. Component internals have no idea of the outside world. All the outside world needs to know is that the component fires this kind of event. For instance, a "Play" button for a custom audio player.

<i class="glyphicon glyphicon-play" on-click="play">Play</i>

Using the component, we can assign handlers to the events fired from the component. In a jQuery manner, we assign eventName[.namespace.subnamespace].

<component on-play="eventName.namespace.subnamespace" />

Listening for the event is as follows. It's similar to jQuery as well.

this.on({
  // Listens to all namespaces for eventName
  'eventName' : function(event,arg1,arg2,...){...},

  // Listens to all subnamespaces for eventName
  'eventName.namespace' : function(event,arg1,arg2,...){...},

  // Listens only to subnamespace for eventName
  'eventName.namespace.subnamespace' : function(event,arg1,arg2,...){...},

  // Listens to any namespace with subnamespace "foo"
  'eventName.*.foo' : function(event,arg1,arg2,...){...}
})

Firing would be similar as well:

// Triggering above
this.fire('eventName',arg1,arg2,...);
this.fire('eventName.namespace',arg1,arg2,...);
this.fire('eventName.namespace.subnamespace',arg1,arg2,...);
this.fire('eventName.someunrelatednamespace.foo',arg1,arg2,...);

// Multiple events - comma separated
this.fire('eventName, anotherEvent, anotherEvent.with.namespace',arg1,arg2,...);

@MartinKolarik
Copy link
Member

@fskreuz

That way, if you borrow components from another project (like we do now in ours), we just slap it in, change the namespace in the parent template, and we're good to go. We need not tinker with the component code.

That's true, but only as long as the "borrowed" component doesn't contain another component:

Component = Ractive.extend({
    template: '<span id="test" on-click="someEvent">click me</span>'
});

Middle = Ractive.extend({
    template: '<component/>'
});

ractive = new Ractive({
    template: '<middle/>', // You can't set a namespace for `someEvent` here.
    ...
});

At least, that's how I understood this so far - that there's only one namespace. Do we want to add a namespace on each level, so that someEvent would become middle.component.someEvent?

<component on-play="eventName.namespace.subnamespace" />

This is basically the same as manually bubbling the events, i.e. what we have now. You don't have to do it on every level, but still...


Thinking about this more, I do agree that adding namespaces on event level wasn't such a good idea. In most cases you'll want to namespace all events with component's name which means you'd have to repeat it all over your template. Adding a namespace attribute on component is probably better.

This brings me back to the question of attribute's name. @martypdx suggested event-namespace, I'd go with simpler namespace.

@fskreuz idea of adding ractive- prefix would be good to distinguish it from regular attributes, but as @martypdx said, we haven't use ractive- prefix so far so to be consistent we shouldn't use it unless we want to use it everywhere.

@martypdx
Copy link
Contributor Author

Okay, another go, thanks for all the feedback thus far. Keep in mind that#1141 makes a number of event "bubbling" use cases obsolete.

I finally hit that point where I started taking options away to reduce complexity, which is usually a good sign. Here's where current code stands:

all events bubble

Outside of the component events bubble as both the provided event name and with the added prefix of the component tag name.

var ractive = new Ractive({
    template: '<widget/>',
    components: { widget: Ractive.extend({ template: '<p on-click="foo"/>' }),
})
// both good
ractive.on('foo', function(){})
ractive.on('widget.foo', function(){})

Inside the component instance itself, only the handler "as named" fires. Likewise for proxy events attached to the template component declaration.

cancel bubbling

The whole adding a method (.stopBubble) thing was a headache as it was forcing the inclusion of an event object for the ractive.fire() case. There's some merit to having the fire case have an ractive-generated event object, but it raised more questions than it answered. Particularly with #1146, what would it mean? Happy to revisit, but hopefully outside of this PR and in a broader context.

There was one idea out of it which I did incorporate. Bubbled events, if they did come from a proxy-event, get a component property added with a ref to the component that fires the event.

ractive.on('widget.foo', function(e){
    // hello component
    e.component.observe('bar', function(){...})
})

So how do you cancel event bubbling? There's two ways, the first is:

return false from the handler

ractive.on('widget.foo', function(e){
    // you no bubble
    return false;
})

Yes, this will cause both the original DOM event to have .stopPropagation and .preventDefault called and the ractive event to stop bubbling up components. But the more I thought about it, so what? Could be wrong, but seems like an edge case that you would care. You can always explicitly call the DOM methods, or refire the event yourself. Or, use the second method:

component proxy-events implicitly cancel bubbling:

<widget on-foo='bar'/>

If you're going to the trouble to change the foo event to a bar event, why would you want foo to continue to fire? With event bubbling, you don't need to manually proxy events at each layer anymore, so it makes sense that they implicitly cancel bubbling.

But how do you cancel all events without having to list them all out? Before answering that, let me get to the next thing:

pattern matching works for events

You can now use same pattern matching we know and love elsewhere in events:

ractive.on('*.foo', function(e){})

There's no "keypath", but it behaves the same way. This means you can create your own namespaces when you declare the events:

<p on-click='timer.start'>start</p>

Or use the component prefixes, or both. The pattern matching is same as observers in that the number of parts has to match. So * will not match foo.event, but *.* will.

Which leads to one last thing:

component proxy events can use wildcards

<widget on-foo.*='bar'/>

(But using them on an element is invalid: <p on-*.bar='handler'/> will throw on debug, otherwise warn and render the listener invalid)

This means you could, in theory, cancel all events using:

<widget on-*-*.*=""/>

@martypdx
Copy link
Contributor Author

I just realized that per @fskreuz 's comment that jquery goes right to left. With ractive keypaths, I'm use to going left to right in the hierarchy, which is why I went that way. Not really attached one way or the other.

@martypdx
Copy link
Contributor Author

Another thing to point out is that the lifecycle events bubble as well. Which means if you do ractive.on('teardown', ...) or ractive.on('change', ...) you'll get all components as well. We could only fire the prefixed version of those events, not really sure.. Reserved event names only bubble under namespace.name.

@fskreuz
Copy link
Contributor

fskreuz commented Aug 31, 2014

@martypdx I never really liked the way jQuery handled namespacing :P ns.subNs.subSubNs.event looks more understandable due to heirarcy.

@martypdx
Copy link
Contributor Author

@fskreuz reading up on jQuery's event namespacing, they think of it more as adding a css class where any namespace can match, rather than a hierarchy: event.ns1.ns2.ns3 matches event.ns1, event.ns2, event.ns3 and event.

@MartinKolarik
Copy link
Member

events bubble as both the provided event name and with the added prefix of the component tag name

How can we subscribe to events from the main instance, without subscribing to events from child components?

var ractive = new Ractive({
    template: '<widget/><p on-click="foo"/>',
    components: { widget: Ractive.extend({ template: '<p on-click="foo"/>' }),
});

// No way to subscribe only to our "foo", not "widget.foo"?
ractive.on('foo', function () {});

I thought this was one of the reasons to implement namespacing.

@martypdx
Copy link
Contributor Author

@MartinKolarik I'm on board with that as I would also prefer that the events not bubble under their name as declared in the proxy event. I think we would be unleashing ͢z̯̘̱̣̣a̙͔͕̦̬͍l̡̖̼̣̥̰̥̗ģ͍͚̖͈̹o͓̩̼̤͖̗̥ .

What if we did bubble declared name if it containes a .. That way component author can created meaningfully namespaced events different than the default component tag name.

var ractive = new Ractive({
    template: '<widget/><p on-click="foo"/>',
    components: { widget: Ractive.extend({ 
        template: '<p on-click="foo"/><p on-click="pickle.berry"/>' 
    }),
});

// handles only foo on top-level instance as expected
ractive.on('foo', function () {});
// these all work to get to component events
ractive.on('widget.foo', function () {});
ractive.on('pickle.berry', function () {});
ractive.on('widget.pickle.berry', function () {});

@MartinKolarik
Copy link
Member

@martypdx

What if we did bubble declared name if it containes a .. That way component author can created meaningfully namespaced events different than the default component tag name.

I would prefer always adding a namespace to bubbled events:

var ractive = new Ractive({
    template: '<widget/><p on-click="foo"/>',
    components: { widget: Ractive.extend({ 
        template: '<p on-click="foo"/><p on-click="pickle.berry"/>' 
    }),
});

// handles only foo on top-level instance as expected
ractive.on('foo', function () {});

// these all work to get to component events
ractive.on('widget.foo', function () {});
ractive.on('widget.pickle.berry', function () {});

// if you want to listen to "pickle.berry" on all components, not only widgets, you can still do this
ractive.on('*.pickle.berry', function () {});

// Is there any other use case I missed?

Deciding whether the event should bubble or not based on it's name doesn't really appeal to me.

@martypdx
Copy link
Contributor Author

@MartinKolarik I'm down with that. Only other thing to add is that you can always change the event anywhere up the stack if you want it to be something different:

var ractive = new Ractive({
    template: '<widget on-foo="flower"/><p on-click="foo"/>',
    components: { widget: Ractive.extend({ 
        template: '<p on-click="foo"/><p on-click="pickle.berry"/>' 
    }),
});

ractive.on('flower', function () {});

So it feels covered to me.

@fskreuz seem to be advocating more for auto-bubbling, but I have not been entirely comfortable with that. I think that {{yield}} is going to diminish need for bubbling events anyway, so I'd like to make this change and get it committed. If people are still writing boilerplate or have other needs, we can always change things.

martypdx added a commit that referenced this pull request Aug 31, 2014
@martypdx martypdx merged commit 58b7125 into dev Aug 31, 2014
@martypdx martypdx deleted the event-bubbling branch August 31, 2014 22:03
@MartinKolarik
Copy link
Member

👍

@Rich-Harris
Copy link
Member

awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants