aurajs / aura

A scalable, event-driven JavaScript architecture for developing component-based applications.
MIT License
2.94k stars 256 forks source link

Pub/Sub as part of core #100

Closed sindresorhus closed 11 years ago

sindresorhus commented 12 years ago

There has been some questions about how to swap out the existing pub/sub implementation:

How does one swap out your existing pub/sub implementation for another one?

What is the benefit of doing something like that?

IMHO, since pub/sub is such an integral part of Aura, wouldn't it be better to focus on making it really good instead of abstracting it away?

addyosmani commented 12 years ago

One of the concepts we initially wished to maintain was making it flexible enough for any piece of the Aura architecture to be swapped out. This covered both the pub/sub implementation we were using as well as the mediator implementation to an extent. Sindre makes an interesting point that perhaps the Pub/Sub we expose is sufficient and should remain something tied to core, that isn't swappable.

I should point out, I believe at the moment it doesn't cost us to make it swappable, but whether we should be documenting this or not is up for discussion.

Does anyone else have thoughts on this?

tony commented 12 years ago

My first impression is also keep our current, What will be the expectation for community-built extensions like backbone-aura? For instance, I'd like to see Backbone.prototype.Views clean up on with our pubsubs.

What are the current pub/subs implementations to look at if one wanted to do some HW?

I know Backbone.Events, Marionette's EventBinder and EventAggregator. A few other frameworks have them. How are they different? Let's see what the delta is, when doing so we'll learn enough about them to either improve Aura's implementation, create an abstraction layer, etc.

addyosmani commented 12 years ago

To an extent, they follow the same pattern, but there are also standalone implementations like AmplifyJS, PubSubJS and a few others implemented on blog posts and the like.

The only place I'd really imagine it (maybe) making sense to swap the implementation out is if you want to say, tie into something specific to a framework. Like, maybe a Backbone developer would want to use Backbone.Events for communication instead. The flip-side of this however is that if our own PubSub comes with Aura and you're a) getting it for free and b) it's not going to return what the Backbone.Events object would, maybe a developer would be better off just sticking to using Backbone.Events for anything in the Backbone extension or their Backbone module and the Aura PubSub for communication between widgets/views/anything else.

rumpl commented 12 years ago

The problem I see if the pub/sub implementation is swappable is that someone who would use another implementation could end up being dependent on that particular implementation doing something specific.

If we do that, there is a big chance that we will no longer have "...modules that can exist on their own or be dropped into other projects..." (quote from here :))

So yeah, I agree with @sindresorhus that it would be better to just make it great and make people use it.

addyosmani commented 12 years ago

^ I'm happy for us to go with that decision. I guess the next question is, what do we think can be improved in our Pub/Sub implementation :)

sindresorhus commented 12 years ago

^ agreed.

Does Aura support namespaced events? Take a look at EventEmitter2. It has some great ideas.

https://github.com/hij1nx/EventEmitter2

addyosmani commented 12 years ago

Goal: Improve our Publish/Subscribe system in core

  1. Figure out a way to support namespaces/special characters in our topic/event names. Although we support using strings as keys in our permissions structure at the moment, we don't seem to be able to use namespaces (e.g a:b) in our topic names. At least my tests so far don't appear to suggest this works at present. (EventEmitter allows you delim namespaces using delimiter: '::' but perhaps we could support a number of cases).
  2. Playing with our Pub/Sub today whilst I found the permissions structure useful for high-level topics/events, I wanted a way to easily re-use the Pub/Sub system without this restriction in two scenarios:

a) Permissions completely switched off. I'd like a way to allow all communications between all widgets if the permissions layer seems overkill for my app.

b) enabled, but having a way to use pub/sub for events within a specific widget. So, lets say that I'm in the calendar widget and I want to be able to publish and subscribe to events within that widget. I'm not going to be touching other modules with this communication but I also dont want to have to enable every event/topic I'm going to publish. It would be nice if there was a way for me to do in-widget Pub/Sub free of permissions.

Maybe there's a way we could enable pub/sub in this case but only for the current context? i.e

sandbox.subscribe(topicName, callback) is supported for in-widget communication but sandbox.subscribe(topicName, detail, callback) requires permissions?

same with publish. sandbox.publish(topicName, data) supported for in-widget, sandbox.publish(topicName, detail, data) requires permissions.

addyosmani commented 12 years ago

Any thoughts on what (if anything) in the above we should implement?

//cc @tony @rumpl @robertd @sindresorhus @gersongoulart

rumpl commented 12 years ago

I am not sure what would the subscription priority be used for. Any use cases ?

Wildcards could be nice, someone wanted to be able to log everything that goes around, subscribing to everything could solve that case.

I agree about the permissions, we should have a way to disable them. They are really useful when you are using someone else's widget in your application.

@addyosmani whait would be in the detail parameter in the sandbox.subscribe(topicName, detail, callback) call ?

We should stop using a simple object ({}) to store the channels. What happens when someone publishes 'constructor' ? Once we do that we could add namespaces or wildcards.

addyosmani commented 12 years ago

To be honest, I haven't seen many P/S systems implementing priority other than Amplify. I guess a use case is that you have a dense UI composed of lots of components with a high level of messaging and you wish to give priority over certain types of communications more than others. imo, its an edge case.

Wildcards would be lovely to have.

RE: detail, it was probably late when I was thinking about it, but it may have been a context. To be honest, I don't mind us implementing something very close to what we have now that would just enable permission-less P/S within a widget itself.

sindresorhus commented 12 years ago

Yes to namespacing and wildcards. I don't see the point of having the namespace delimited configurable though.

No to priority. I can't think of any the real-world use-case.

Btw. Any reason publish/subscribe is used instead of pub/sub or emit/on?

addyosmani commented 12 years ago

I think when we started the project a year ago that the on and emit trend was only starting to kick off and we were waiting to see whether it got more broadly adopted. I personally wouldn't mind if we switched to them.

++ to no on priority and yes on namespacing + wildcards.

addyosmani commented 12 years ago

If everyone is in agreement, lets do namespacing, wildcards and the change to emit/on.

On wilcards, are the cases we want to support:

?

The first is relatively easy to implement as a catch-all. I'm guessing we could use some regex-fu on the second. Any other cases?

Hmm. Namespaces. So, I haven't written a Pub/Sub solution before that supports namespacing in topics so I'm wondering whether this is something we implement as a generalised way to include special characters in a topic-name or make it more specific than that.

Any opinions on this would be appreciated before we go ahead and implement.

sindresorhus commented 12 years ago

I haven't written a Pub/Sub solution before that supports namespacing in topics so I'm wondering whether this is something we implement as a generalised way to include special characters in a topic-name or make it more specific than that.

Me neither, but could just borrow some ideas from EventEmitter2 or even jQuery.

addyosmani commented 12 years ago

Renamed publish to emit and subscribe to on everywhere I could find in https://github.com/addyosmani/aura/tree/pubsubv2. Would anyone mind taking a look?

Whilst going through, I noticed that (unless I'm missing it because its late) we don't seem to offer a direct unsubscribe or off method. Is this worth implementing as off to complete the set?

addyosmani commented 12 years ago

Simplistic wildcard support based on how EventEmitter2 handles the * case https://github.com/addyosmani/aura/blob/9a0bb091dae0eb510272411948460c8bee47cbf3/src/aura/core.js#L133. This doesn't take care of wildcards in namespaces, but need to implement namespace support first before that can be tackled. I would have thought there would be a more elegant solution to the wildcard solution but a few people seem to be doing something similar.

tony commented 12 years ago

@addyosmani: looking good. Tested in grunt and firebug.

I wonder if we could .split pubsub at : or .? Then people could perhaps chain to their hearts content. Hehe, you know what would be cool, if we took at look at borrowing from https://github.com/isaacs/minimatch.

This is kind of free-flowing, aura doesn't spec to this but could allow people to do a:

obj.emit('nav.link.click')
obj.emit('nav.button.click')

obj.on('nav.*.click')

or

obj.emit('nav.*.enable')

obj.on('nav.buttons.enable')
obj.on('nav.search.enable')

There is also a ** globstar that could allow obj.emit('nav.**.enable') to hit obj.on('nav.search.input.enable')

rumpl commented 12 years ago

Hey @addyosmani, with your implementation, calling sandbox.on('*', cb); will subscribe only to the existing channels, not those to come. Is this what you wanted ?

addyosmani commented 12 years ago

@tony We could! I would love for us to solve namespacing sometime soon and I absolutely love the suggestions you made of how and where wildcards could fit in. minimatch (just looking at it for the first time) seems like a fairly heavy implementation for client, but there may well be some nice regex in there we could use. Would you be interested in taking a look at implementing a POC of namespaced topics? :)

@rumpl Good catch! I'd thought about this use case when I was implementing but figured we'd solve it later, however I do very much want to support both existing and future.

If there are no further comments on the emit/on changes, I can land those so we can continue iterating on the rest of this ticket without rebasing on branch.

Did anyone else have comments on unsubscribe?

rumpl commented 12 years ago

Hello all,

You can find here (it's on the ns-wildcard-pubsub branch) a working POC of what we are talking here. I don't have the time to do the core.stop but it is pretty easy to do.

So, we have:

You can do something like this :

sandbox.subscribe('test.*', 'todos', function() {
  console.log('YAY');
});

sandbox.publish('test.itworks', 'todos');
sandbox.publish('test.itworksagain', 'todos');
sandbox.publish('test.dont.yay', 'todos');

And you will see two YAYs in the console.

Just realized I used the master and not your branch with on/emit... Sorry about that.

addyosmani commented 12 years ago

@rumpl That is so awesome. Nice work! I'm sure we can iterate on it a little further but I'm really stoked to see what you have in place now working. I'll play around with it further.

Re: master vs branch - no worries!. I can rebase my branch before landing. I'm more excited to see the above land :)

tony commented 12 years ago

@rumpl Clutch, man 8-) @rumpl, @addyosmani, @sindresorhus : This is a good example of where new functionality should be backed tests before merge. @rumpl's YAY trails seem like a test in itself :)

I'll play w this branch more too.

rumpl commented 12 years ago

Hey all, thanks for the heads up. Apreciate it.

Indeed, it is a very good examaple of the kind of functionality that needs thorough testing. That is way I didn't do a PR. I wanted to see if everybody was OK with what I was doing. And of course, I would love if you guys could take a good look and help me with the tests and bugfixes.

It is just a POC for now, meaning the code is probably bad and could be enhanced so any help is welcome :)

Sent from mobile - please excuse tone and brevity

On 10 sept. 2012, at 20:00, Tony Narlock notifications@github.com wrote:

@rumpl Clutch, man 8-) @rumpl, @addyosmani, @sindresorhus : This is a good example of where new functionality should be backed tests before merge. @rumpl's YAY trails seem like a test in itself :)

I'll play w this branch more too.

— Reply to this email directly or view it on GitHub.

tony commented 11 years ago

Pertinent reading on namespaced / wildcard pubsub:

There is a codereview of EventEmitter2 at http://dailyjs.com/2011/09/19/code-review/. EventEmitter2 is based on a conversation about namespaced events.

addyosmani commented 11 years ago

Today I merged in a few changes we've been discussing lately including very basic wildcard support, emit/on renaming etc. I still need to go through the work by @rumpl and bring it in as a PR or something that's had a code review done to it. @rumpl would love for you to get the credit for the work. Would you mind doing a PR so that we can leave inline code comments and iterate on? :)

rumpl commented 11 years ago

Sure, I just have to update it to the new emit/on function names. Will try to do it tonight.

Thanks.

addyosmani commented 11 years ago

@rumpl think you might have a chance to look at this again? :)

atesgoral commented 11 years ago

Should there be a mechanism for the emitter of an event to optionally consolidate feedback from consumers? PubSub is a great mechanism for agnostic collaboration among modules, but the Service Locator pattern (as we see in Eclipse, Android, etc.) goes a further step by allowing information collection, in addition to broadcasting.

Use case: A navigation module that queries the service locator for other modules that would like to contribute navigation items. It's awkward doing this with pure PubSub eventing (or I'm missing the ideal solution).

One can always implement a feedback consolidation mechanism over PubSub (for example by making sure the event is always consumed synchronously, and then attaching some kind of callback to the event payload, and praying for consumers to call your callback with their data). But I just feel that it would be nice to see a generic mechanism baked into this framework...

addyosmani commented 11 years ago

@atesgoral I'm a little silly in that I think better in terms of code. Do you think you might have time to throw together a proof of concept / PR that introduces what you're describing so that we can play around with it and discuss how this might work its way into the current PubSub system?

atesgoral commented 11 years ago

@addyosmani I could surely give it a shot!

Actually, @gorillatron's sandbox.requestInterface idea comes close to what I was mentioning:

sandbox.registerInterface("data::users", users);

// then...

sandbox.requestInterface("data::users", callback);

I'm merely taking it a step forward to allow for multiple results (Array) to be returned.

solenoid commented 11 years ago

@addyosmani I did have a comment on unsubscribe / off being put in to complete the set. I've centralized all the pub / sub needs through the one provided by aura. For high widgets this has been working great, because they start and stop and that unsubscribe is handled in the stop. But for other lower level uses specifically around data handling I just came into a need for very short lived on handlers that will be taken off for success or failure.

It seems pretty straight forward to tease this capability out of stop in core, but I'm not as comfortable on specifics of where it should wash out to. I'd appreciate any clarity on if this is going to land or if I can help out.

addyosmani commented 11 years ago

@solenoid Thanks for your interest. Could you clarify what you meant by wash out to? I would place the off behaviour in core to accompany on and emit.

solenoid commented 11 years ago

By 'wash out to', I meant how it would integrate into the sandbox specifically. I like many aspects of the pub / sub channel being wrapped over there but have found myself transposing arguments around permissions often so I'm less comfortable in the sandbox.

Also I wanted to let you know that using aura at my place of work is going great and in general has helped conceptually and with the quality of code at the same time.

addyosmani commented 11 years ago

I feel like we've addressed this (or rather, are addressing this) as a part of the aura-express work landed in master. Closing for now.