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

Component events should bubble to parent instance #782

Closed
kvnneff opened this issue Jun 3, 2014 · 5 comments
Closed

Component events should bubble to parent instance #782

kvnneff opened this issue Jun 3, 2014 · 5 comments
Milestone

Comments

@kvnneff
Copy link

kvnneff commented Jun 3, 2014

I came across #477 but I think its worthwhile to keep an issue open for this. Despite it not technically being a "bug" it still feels broken to me to, essentially, be restricted to assigning event handlers in my templates rather than programmatically.

Using package installers like Component.io with self-contained Ractive components makes it easy to wrap up a generic Ractive component that can be dropped in and out of projects with ease, and can contribute towards building small libraries of reusable widgets for teams.

This gets cumbersome, however, when someone wants to drop in a widget that, say, may perform four different operations on a set of data. In order for that widget to be robust it should this.fire() a custom event every time it performs an operation, and most likely it should emit the data that has been operated on.

Now anyone wanting to use my widget would have to:

<widget onThis="doThis" onThat="doThat" onAnd="doAnd" onTheOther="doTheOther">

as well as:

ractive.on({
    this: doThis,
    that: doThat,
    etc....

Integrating the new component doesn't seem so fun!

Let me do this instead (contrived example):

var ListWidget = require('ractive-components-companyListWidget'),
    Ractive = require('ractive'),
    messenger = require('messenger'),
    widgetConfig = {},
    ractive;

widgetConfig.sortField = 'Name';
widgetConfig.title = 'Some List of Data';

ractive = new Ractive({
    components: {ListWidget: ListWidget},
    data: {companyListWidget: widgetConfig}
});

ractive.on('companyListWidget:add', messenger.send);
ractive.on('companyListWidget:remove', messenger.send);

And drop it in:

<div>
    <ListWidget>
</div>

I really appreciate Ractive so I may even look into this myself when I have some free time. For the moment, though, it sticks out to me as something Ractive got pretty wrong, but only because everything else seems so right.

@martypdx
Copy link
Contributor

martypdx commented Jun 3, 2014

Since I put in issue #477, I'll say my thinking has progressed somewhat on this. Things get simpler if instead of trying to relay events, you just listen on the component. Your example isn't really much different than doing:

ractive = new Ractive({
    components: {listWidget: ListWidget},
    data: {companyListWidget: widgetConfig}
});

ractive.findAllComponents('listWidget').forEach( function(listWidget) {
    listWidget.on('add', messenger.send);
    listWidget.on('remove', messenger.send);
});

Except that you have to iterative the component list (and track adds and removes on your own).

Maybe what's need is a method on a component list that takes care of that component .on() and list management:

// with { live: true }, ractive will already keep this list in sync
var listWidgets = ractive.findAllComponents('listWidget', { live: true })

// this means listen to all the components in the list
// because {live: true } above, ractive will take care of .off() 
// and .on() for components that get added/removed to the list.
listWidgets.on('add', messenger.send);
listWidgets.on('remove', messenger.send);

@martypdx
Copy link
Contributor

martypdx commented Jun 3, 2014

I do like your idea of introducing what amounts to event name spacing similar to jQuery and others. Though I would stick with a . instead of :

ractive.on('companyListWidget.add', messenger.send);
ractive.on('companyListWidget.remove', messenger.send);

And because it goes through multiple levels for which the parent component may not know the alias, we might have to use the Component class:

ractive.on('ListWidget.add', messenger.send);
ractive.on('ListWidget.remove', messenger.send);

This would be even less work (though it would need all the functionality) as you don't have to find the components yourself).

Though, how to namespace (alias or class names) worries me as there a cons to both approaches.

@jonvuri
Copy link
Contributor

jonvuri commented Jun 3, 2014

@staygrimm
Personally I like having to explicitly declare all proxy events in a Ractive template, even those coming from components. It keeps everything simple and encapsulated. I also don't like the idea of having to perform more setup on components than just require()ing them and dropping them in.

Also also, in your example how is the companyListWidget namespace being tied to the ListWidget? It seems like you need a way to assign a namespace to each widget, in which case you're not really saving a lot of complexity or boilerplate, only introducing more.

@jonvuri
Copy link
Contributor

jonvuri commented Jun 4, 2014

@martypdx
I'm confused about your iteration examples. Wouldn't it be simpler to have each instance of the component fire the same proxy event, if the handler is always the same?

@martypdx
Copy link
Contributor

martypdx commented Jun 4, 2014

@jrajav I'm subscribing to the component event directly on each instance.

<listWidget on-add='handler'/>

is more or less same as:

<listWidget/>
ractive.findComponent('listWidget').on('add', handler)

The iteration example is just for multiple components.
The whole finding thing is a PITA, but for nested components it can be way better than having to re-proxy the event at each level.

We currently declare events in template(s) (or fire them programmatically from components) and subscribe to them like:

ractive.on('event', handler)

If we hid the complexity of finding the components and used namespaces, it would behave same as above, just with the component namespace:

ractive.on('listWidget.add', handler)

Just an idea, not attached to anything...

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

No branches or pull requests

4 participants