ideas for v2.0 (if it ever happens)
millermedeiros opened this issue · 16 comments
signals is being used by many projects, and API been stable since forever, but somehow I think some design decisions weren't as good as they could; mainly because I was following AS3 Signals API and also because I added features just because they were easy to implement but without a real reason to do so (which was a big mistake).
here is a list of things that I would probably do differently if I was rewriting the project from scratch today (mostly to simplify things):
rename add and addOnce to listen and once
IMO foo.clicked.listen(doSomething) is better than foo.clicked.add(doSomething).
remove Signal#remove
if user plans to remove the listener afterwards he can use the SignalBinding#detach instead.
var binding = foo.clicked.listen(doSomething, this);
// ...
binding.detach();this will avoid cases where user forgets to pass the context on Signal#remove; would force user to structure code in a better way, and would also simplify some of the internal logic.
rename SignalBinding#detach
binding.detach() is kinda weird.. maybe a better name would be binding.cancel().
remove Signal#has
since we won't support Signal#remove this should also be removed.
dispatch a single argument
this would encourage users to use objects instead of multiple arguments. (which are easier to extend in the long run and favors better practices).
remove SignalBinding#params
instead of doing binding.params = ['foo', 1] it would be better to add a special property to map the dispatched value and use a setter for it.. maybe something like binding.map(setDefaultArgs) which would be more flexible and cleaner.
in fact it would be better to kill this feature.. user can simply create a new function that process the data before sending to original method. - this feature was mainly added because of my SectionController and it's a bad practice.
remove SignalBinding getters (isOnce, isBound, getSignal, getListener)
these are rarely needed and user could simply access the pseudo private properties. - if your app needs this logic you are probably doing something wrong.
remove Signal#getNumListeners
this seems unnecessary. (maybe just a simplehasListeners()?)
convert all properties into getters/setters
too easy to mistype active and memorize. better to use pause(), resume() and for memorize to be set on the constructor and not able to be changed at runtime: new Signal({memorize: true})
Signal#removeAll??
unsure if we should keep/rename this.
A way to listen when handlers are added/removed
In some cases I want to defer some heavy work until there is someone actually listening for changes.. so it would be good to have a simple way to listen when listeners are added/removed from the Signal. - Maybe just when add is called when Signal have no listeners or when remove removes the last listener.
binding.detach() is kinda weird.. maybe a better name would be binding.cancel().
As you're going for listen. stopListening might be a better name (like Backbone done it).
Hi - thanks for porting Signals to JS. Nice work.
I'm not sure if you're still considering this topic, but some feedback on the API change ideas. Listen doesn't make as much sense to me. It reads like the signal is to listen to the listening function. IMO it is clearer that the listening function is added to the signal bindings.
I love signals. It has the best API concept of all that I've seen. But there is one lacking feature that I would love to see. Facebook's flux pattern introduces a "dispatcher" concept. JS-signals could almost follow that role except for the "dispatcher.waitFor()" method. You can read about it here (https://facebook.github.io/flux/docs/dispatcher.html). It may be taking JS-signals in a direction that you don't want it to go, but I think it would be a nice addition.
@explorigin just saw the waitFor implementation and it's really weird.. If I need things to happen in sequence I would probably just use a single handler that call the functions in sequence (better IMO) or set a different priority when adding the listeners.. but I'll try to understand better why Flux decided to add this and consider something similar for the future.
Looks like the concept behind the Flux Dispatcher is a queue with a
buffered input: the next payload won't be processed until all the callbacks
have processed the current payload.
The concept behind waitFor looks like is to reorder the queue of
callbacks in one dispatch cycle to guarantee the required order of payload
processing. The classic pub-sub does not allow to define the order of
callback execution: the calls go in the order the components have
subscribed to the event which can be inconsistent (depends on the order of
components loading and instantiation, e.g. with async loading or lazy
instantiation).
These are my own observations, I may be wrong, just wanted to share my
thoughts.
Looks good;
removeAllcould indeed be removed; it's invasive and personnaly never had a use case for it.addcould simply return an unsub function ? SignalBinding seems like an implementation detail that shouldn't be exposed.
@AlexGalays removeAll is required to implement a destructor in a component that owns the signal instance (it could be reworked to be a true destroy that forgets all the handlers, releases all the resources it takes and marks itself as a destroyed object to avoid add-ing more handlers to it later).
I think clicked.once().then(event => doSomething()) would be nice (basically, signal.once() returns a promise which is fulfilled the next time the event happens. Returning a promise here would be more flexible than signal.listenOnce(doSomething).
Also, my suggestions for the api in general:
var myObject = {
clicked: new signals.Signal()
};
// listening to a signal
var handle = myObject.clicked(doSomething)
// emit a signal
myObject.clicked.emit({some: 'thing'})
// listening for the next signal
var promise = object.clicked.once() // or object.clicked.next()
promise.then(event => doSomething())
// destroying a handle (remove listener)
handle.destroy()
// remove all listeners
myObject.clicked.destroy()I agree with@AlexGalays about SignalBinding being something user shouldn't be bothered with.
However I don't see why Signal#remove should be removed.
Actually at the contrary I came to this thread because I would love an enhancement mimicking the functioning of Backone.Events#off (http://backbonejs.org/#Events-off). One could remove a callback by giving the callback, the callback and the context or only the context (which removes all callback for the given context, very useful as callback can be provided anonymously).
Personally, I never experienced the multi-context problem you were talking about.
Just a comment that I would love to see a new version. One thing I would very much like to see in a 2.0 would be performance testing and optomization. For example I suspect dispatch is not optomized in v8: http://www.slideshare.net/up2soul/planet-html5gameengine-javascript-performance-enhancement/9
So what's going on with this project? I'm actually looking for a simple event library as I'm not using Crossroads.js and Hasher in a new project. I'd be willing to contribute!
@divmgl I created an alternative version (https://github.com/Hypercubed/mini-signals) that implements some of the v2.0 features discussed here. I have contacted @millermedeiros to ask if he would like to merge or perhaps converge on an API, but I haven't heard back.
@Hypercubed sorry for not replying to your messages.. I'm moving next week and having to deal with a lot of paperwork and other assorted things for the move.. I did not had time to look at your implementation, yet.
I kinda stopped changing signals because I believe there are many projects relying on the current API/behavior.. That's why I never spent time implementing the features/changes highlighted above.
@millermedeiros I completely understand... on both points.
Hey @Hypercubed I like your library. I will continue discussion there. @millermedeiros Signals is actually working fine, I was just wondering about the state of the project. Thanks!