ideas for v2.0 (if it ever happens) #60

Open
millermedeiros opened this Issue Dec 26, 2013 · 16 comments

Comments

Projects
None yet
10 participants
@millermedeiros
Owner

millermedeiros commented Dec 26, 2013

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.

@SBoudrias

This comment has been minimized.

Show comment
Hide comment
@SBoudrias

SBoudrias Jan 17, 2014

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).

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).

@db

This comment has been minimized.

Show comment
Hide comment
@db

db Jun 10, 2014

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.

db commented Jun 10, 2014

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.

@explorigin

This comment has been minimized.

Show comment
Hide comment
@explorigin

explorigin Oct 31, 2014

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.

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.

@millermedeiros

This comment has been minimized.

Show comment
Hide comment
@millermedeiros

millermedeiros Oct 31, 2014

Owner

@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.

Owner

millermedeiros commented Oct 31, 2014

@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.

@sompylasar

This comment has been minimized.

Show comment
Hide comment
@sompylasar

sompylasar Oct 31, 2014

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 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.

@AlexGalays

This comment has been minimized.

Show comment
Hide comment
@AlexGalays

AlexGalays Jan 11, 2015

Looks good;

  • removeAll could indeed be removed; it's invasive and personnaly never had a use case for it.
  • add could simply return an unsub function ? SignalBinding seems like an implementation detail that shouldn't be exposed.

Looks good;

  • removeAll could indeed be removed; it's invasive and personnaly never had a use case for it.
  • add could simply return an unsub function ? SignalBinding seems like an implementation detail that shouldn't be exposed.
@sompylasar

This comment has been minimized.

Show comment
Hide comment
@sompylasar

sompylasar Jan 11, 2015

@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).

@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).

@speigg

This comment has been minimized.

Show comment
Hide comment
@speigg

speigg Feb 5, 2015

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).

speigg commented Feb 5, 2015

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).

@speigg

This comment has been minimized.

Show comment
Hide comment
@speigg

speigg Feb 5, 2015

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()

speigg commented Feb 5, 2015

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()
@QuentinRoy

This comment has been minimized.

Show comment
Hide comment
@QuentinRoy

QuentinRoy Feb 11, 2015

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.

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.

@Hypercubed

This comment has been minimized.

Show comment
Hide comment
@Hypercubed

Hypercubed Jun 4, 2015

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

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

@divmgl

This comment has been minimized.

Show comment
Hide comment
@divmgl

divmgl Sep 16, 2015

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 commented Sep 16, 2015

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!

@Hypercubed

This comment has been minimized.

Show comment
Hide comment
@Hypercubed

Hypercubed Sep 17, 2015

@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.

@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.

@millermedeiros

This comment has been minimized.

Show comment
Hide comment
@millermedeiros

millermedeiros Sep 17, 2015

Owner

@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.

Owner

millermedeiros commented Sep 17, 2015

@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.

@Hypercubed

This comment has been minimized.

Show comment
Hide comment
@Hypercubed

Hypercubed Sep 17, 2015

@millermedeiros I completely understand... on both points.

@millermedeiros I completely understand... on both points.

@divmgl

This comment has been minimized.

Show comment
Hide comment
@divmgl

divmgl Sep 18, 2015

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!

divmgl commented Sep 18, 2015

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!

@Hypercubed Hypercubed referenced this issue in Hypercubed/mini-signals Sep 18, 2015

Closed

Finalize API and release 1.0.0 #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment