Hypercubed / mini-signals

signals, in JavaScript, fast
MIT License
120 stars 12 forks source link

Finalize API and release 1.0.0 #1

Closed Hypercubed closed 9 years ago

Hypercubed commented 9 years ago

I would like to finalize the API and release a 1.0.0 version. The current mini-signals API matches js-signals:

var signal = new MiniSignal();

signal.add(callbackFunc, callbackContext);
signal.dispatch();
signal.remove(callbackFunc);  // or signal.remove(callbackFunc, callbackContext);

My proposal for the first 1.x release is to return the binding object with a detach method:

var signal = new MiniSignal();
var handle = signal.add(callbackFunc, callbackContext);
signal.dispatch();

handle.detach();

I'm looking for feedback.

Hypercubed commented 9 years ago

Also considering signal.once for 1.0.0.

samuelmesq commented 9 years ago

+1 for signal.once

Hypercubed commented 9 years ago

Some version of the second API is the direction I am going to go. This makes detaching a listener O(1). I have merged the new API into master to help the discussion. Still not published to NPM and still room to discuss. Alternative APIs could be:

var signal = new MiniSignal();
var handleDetach = signal.add(callbackFunc, callbackContext);
signal.dispatch();

handleDetach();

Similar to angular's scope.$on. Or perhaps:

var signal = new MiniSignal();
var handle = signal.add(callbackFunc, callbackContext);
signal.dispatch();

signal.detach(handle);

Which can still be O(1).

Hypercubed commented 9 years ago

@samuelmesq .once is now on master. Thanks.

Hypercubed commented 9 years ago

Other choice to make before 1.0.0 is to finalize the names of all the methods:

signal.add could be .addListener, .on, subscribe, forEach. signal.once could be .addOnce, .then. signal.dispatch could be .emit, trigger, .next. signalHandle.detach could be .off.

Other ideas?

samuelmesq commented 9 years ago

it's a matter of taste, i prefer short names like add, once, emit and off. i use these in one little event-emitter i wrote to start my project. (planning replace it, was how i found yours).

definitely add over on, because i think make more sense in reading. in my case i use string for events name, so: ee.on("thisevent", thishandler);

but signals dont have a event name, is kind of weird read that way: mySignal.on(thishandler);. Add a handler is better.

about detach could be also remove, both are ok.

like i said matter of taste.

Good Luck, have fun!

robotmayo commented 9 years ago

I think detach should be remove as remove is often used as the reverse of add

Hypercubed commented 9 years ago

I'm leaning towards:

var signal = new MiniSignal();
var handle = signal.add(callbackFunc, callbackContext);
signal.once(callbackFunc, callbackContext);
signal.emit();

signal.remove(handle);  // or handle.detach();

Is it good to have both methods to detach signal.remove(handle) and handle.detach()?

Also, still debating if signal.add should return a function or an object. If it returns the binding object then remove can have dual purpose of removing by handle (an O(1) operation) or callback ( O(N) ).

samuelmesq commented 9 years ago

what the purpose of handler? i see possibilities, but not practical use cases.

Hypercubed commented 9 years ago

Currently the signal.remove(fn, ctx) method removes all listeners that match the callback and (optionally) ctx. This operation is O(N). I think this method should be discouraged or removed entirely.

signal.add currently returns the listener "handle" with a handler.detach method. This detaches the exact listener that was added. This is a O(1) operation (given the fact that I use a doubly linked lists for handler nodes).

Alternatively signal.add can simply return the detach function.

The advantage of returning a handler object vs. an unsubscribe function is that we can implement both handle.detach() and signal.remove(handle), both O(1). I'm also considering if it is ever useful for a user to modify the callback or the context after it has been added to a signal... or maybe something more crazy like handle.emitFromHere() or handle.pause().

samuelmesq commented 9 years ago

@Hypercubed i get it, return the handle object is cool.

shergin commented 9 years ago

I like naming in the first post, add and detach. detach should not be remove because this is called on different kind of object. Also I like handle concept, because both of complexity (constant time) and clarity.

Hypercubed commented 9 years ago

Thank you @samuelmesq and @shergin. I think remove will be removed (no pun intended) in the 1.0x. I'm leaving it in for the moment until I can do a major bump.

r4j4h commented 9 years ago

The distinction between returning an object containing a reference to the function or directly returning a reference to the function is so difficult!

I really, really want to side with the object because of your point in your last paragraph in this post that we get two implementations for detaching signals.

But then I kept asking myself why do we want to have two ways of doing it? What are the benefits or how they differ?

In one we need the handle reference, so we have to hold on to that when we add our signal.

In the other we need the signal and the handle's references. We likely already had access to the signal still, and so we are in the same position as the first with no real benefit other than explicitly re-stating the signal this handler is from.

But if handlers are outputs from adding functions to signals, that distinction seems useless - there is no chance of a handler being used by multiple functions OR multiple signals as they are already one to one with the function attachments to the signal. The original js-signals implementation, while slower, does not have this distinction through the deprecated functionality: there is the chance that a function reference is added to the same signal multiple times -- in this case the ability to pass a function and a context can aid in distinguishing between these re-used adds.

So that makes me think that it should just stay as simple as possible and just return the detach function directly.

The other uses of such a returned object would totally change my thoughts - e.g. signalHandler.pause assuming it pauses only that handler while allowing others to continue. I'm not sure if this is where you plan to take mini-signals though so I am erring on the lean side of things.

Hope this is helpful and not just me rambling, because now that I've typed it all I fele like I'm just rambling.

Hypercubed commented 9 years ago

@r4j4h Thank you very much for you input. I feel similar. I really want to return the handler object because it seems to have potential (e.g. .pause). But right now it's just an extra JS object that gets created and needs to be garbage collected later.

jhusain/observable-spec spec suggests that "subscribe must return a subscription object.". But for some reason zenparsing/es-observable returns an unsubscribe function.

One more thing I've been thinking of, that might have some impact on this decision, is the .listeners method. Currently, this returns an array of listener callback functions. I don't see the use of that, especially after .remove(fn, ctx) is deprecated. If anything it should return an array of handle objects or an array of unsubscribe functions... which is weird.

Hypercubed commented 9 years ago

Another thought... returning the signalHandler would allow:

var signal = new MiniSignal();
var handle = signal.add(callbackFunc, callbackContext);
signal.addAfter(handle, callbackFunc, callbackContext);
signal.addBefore(handle, callbackFunc, callbackContext);

or perhaps:

var signal = new MiniSignal();
var handle = signal.add(callbackFunc);
handle.addAfter(callbackFunc, callbackContext);
handle.addBefore(callbackFunc, callbackContext);

I've been considering if and how to implement priority. This would allow priority and still be O(1) on add. I'll probably add an .addFirst method at least.

r4j4h commented 9 years ago

It's interesting to see other libraries are already inconsistent in this regard. I like the ideas in observable-spec and at a glance I feel returning the handler object would conform to that which would be nice.

I think you are right, .listeners could be deprecated. Especially if an unsubscribe function is all that is returned.

But I really like that addAfter/addBefore concept! Using the handlers for priority control is clever, but it makes me wonder then if getting a list of handle objects might be useful. Maybe .listeners would work in that case? Maybe .handlers would be a better semantic name? Either way, I can see it being useful to have a way to check the flow of a signal by seeing the order of all the handlers.

divmgl commented 9 years ago

Purely semantic and minor, but maybe we should rename add to listen for clarity. Or at least provide a prototype.listen = prototype.add. I'd be willing to help with the development. Do we have a roadmap?

Hypercubed commented 9 years ago

@divmgl Thank you for your input. add vs listen may be a difficult choice.

I don't have a roadmap. This project started out as a minimal js-signals implementation to address v8 deoptimization (https://github.com/millermedeiros/js-signals/issues/67). When I switched to linked lists I realized it was easy to implement some of @millermedeiros' v2.0 notes (https://github.com/millermedeiros/js-signals/issues/60).

Here are my thoughts:

Hypercubed commented 9 years ago

I'd also like to keep the option of merging into js-signals open.

Hypercubed commented 9 years ago

For curried parameters I'd prefer signal.add(fn.bind(ctx, 'testing', 'testing'));. This becomes more reasonable when we deprecate .remove as there is no API to remove a listener by function.

That makes me wonder is there any difference between signal.add(fn, ctx) and signal.add(fn.bind(ctx));?

millermedeiros commented 9 years ago

I also like the idea of removing the context argument on add.. Nowadays all the important environments supports Function#bind and arrow functions are also becoming available and widely used.

Need to remind that js-signals started back in 2010 and v1.0 was released on 2012... It was different times.

I really like the idea of keeping the API/features as simple as possible.

Also as a background story.. I had a branch that used linked lists (and was theoretically faster) but I decided to use arrays since on my projects it was never the bottleneck (most cases I have 1-2 listeners per event type) and implementation was way more complex. On Sep 18, 2015 22:10, "Jayson Harshbarger" notifications@github.com wrote:

For curried parameters I'd prefer signal.add(fn.bind(ctx, 'testing', 'testing'));. This becomes more reasonable when we deprecate .remove as there is no API to remove a listener by function.

That makes me wonder is there any difference between signal.add(fn, ctx) and signal.add(fn.bind(ctx));?

— Reply to this email directly or view it on GitHub https://github.com/Hypercubed/mini-signals/issues/1#issuecomment-141605853 .

Hypercubed commented 9 years ago

Hi @millermedeiros, really great to have your input. Different times indeed.... in 2010 my primary language was FORTRAN. Fell free to interject as much as you would like. I'd also like to keep this API simple (after all I called it mini-). If later someone wants to make a more robust version they can add back someone of these features (e.g. auto rebind).

In version 0.0.1 I was using some of the optimizations techniques from EventEmitter 3... then I realized that they basically implementing a very dumb (the algorithm, not the people) linked list. It was effectively a linked list for n < 2, then created an array. I tried just an LL and the difference was 2x in the benchmarks, real world applications will vary of course.

Hypercubed commented 9 years ago

I think at some point I'll create a wiki(s) page covering the API difference between mini-signals (pending v1.0) and js-signals. So far I have:

anything I'm missing?

Hypercubed commented 9 years ago

Hello all, thank you for your help. 1.0.0-beta is now on github in the 1.0.0 branch and on npm (npm install mini-signals@1.0.0-beta).

Hypercubed commented 9 years ago

Hello all, thank you all again for your feedback. I have published v1.0.0 (actually v1.0.1 because my NPM foo is bad). This implements all the new API features we discussed and removes deprecated js-signals compatible API. Releasing v1.0.0 means that I have committed to the API and won't make breaking changes unless absolutely necessary. I'll try to follow semver ([breaking].[feature].[bugfix]) from here on out.

The API details and a comparison of mini-signals to js-signals on the wiki.

divmgl commented 9 years ago

@Hypercubed I love your library. I'm currently using it in a project for an abstract router. As soon as I'm done with the implementation I'm going to make it public. In the meantime, I will add you as a contributor. Seriously, great work.

Hypercubed commented 9 years ago

@divmgl, thank you. Maybe the first usage in the wild! I see you are using mini-signals in ES6 through jspm. This is something I struggled with. I would have liked for JSPM users to be able to use the ES6 modules directly, but this would have meant, in some edge cases, ackward syntax like require('mini-signals').default. After a few back and forths with @guybedford here the best I could do was expose the CJS to JSPM. Open an issue if you think this should be rethought.

Hypercubed commented 9 years ago

I'm thinking of following ericelliott's advice and dumping the class syntax for MiniSignal in favor of a factor. Thoughts?

Pro factory: No new keyword (but backwards compat), more composable. Con factory: No instanceOf (breaking change), more verbose code.

Thoughts?

divmgl commented 9 years ago

How would the syntax change?

Hypercubed commented 9 years ago

Instead of:

var MiniSignal = require('mini-signals');
var mySignal = new MiniSignal();

You would use:

var miniSignalFactory = require('mini-signals');
var mySignal = miniSignalFactory();

(of course MiniSignal -> miniSignalFactory is optional).

Here is some relevant links: https://medium.com/@_ericelliott/why-hate-on-class-inheritance-4c1067db3f03#6bc5

divmgl commented 9 years ago

I'd say let's not over-complicate things. I like the regular signal syntax. It gives us the option of creating a factory implementation if we choose rather than having the factory pattern forced on us.

Hypercubed commented 9 years ago

I'll leave it as is for now since it is a breaking change for anyone relying on instanceof but something to think about. Signals are designed to be attached to objects, what is the use case for composing or inheriting from a signal?

divmgl commented 9 years ago

Hey @Hypercubed I'm going to open a separate issue for the factory implementation

divmgl commented 9 years ago

Congrats on 1.0, this is a really solid library

Hypercubed commented 9 years ago

Thanks, feel free to keep discussing... I just wanted to see 100% on the v1.0.0 milestone!