As per @isaacs and @ry's request, changes to pull request for events #1450

Closed
wants to merge 6 commits into from

1 participant

@0x00A

Trailing whitespace removed, named functions, internally used functions privatized (reduced surface area), onAny returns this, doc lines wrapped

There was an interesting issue that was raised during the last pull request's discussion. That was hey buddy, -1 for the micro-optimization. This is referring to some of the functions checking for instance._events -- after thinking about it, i realized that this was an artifact of the ctor not being written in js. There is a marginal gain by reducing this boolean check and moving the assignment to the ctor, but this breaks most of the code base because most of the codebase assumes an empty ctor.

@tj tj and 2 others commented on an outdated diff Aug 4, 2011
doc/api/events.markdown
server.on('connection', function (stream) {
console.log('someone connected!');
});
+
+#### emitter.onAny(listener)
@tj
tj added a note Aug 4, 2011

would this be equivalent to emitter.on('*', fn)? I think that would be nicer than adding another method (personally), less api

@0x00A
0x00A added a note Aug 4, 2011

It could be documented better. * means anything that has a single space, for instance * would fire the events the bazz and beer, but not foo.bar.

@tj
tj added a note Aug 4, 2011

gotcha. I thought one * would just match everything

@koichik
koichik added a note Aug 4, 2011

I also misunderstood.
By default, we cannot use wildcard but we can use onAny().
Okay, looks nice.

@tj
tj added a note Aug 4, 2011

I can see the logic when other segments are present but I still thing "*" only should be everything and then there's no need for this method and unAny which is super awkward

@0x00A
0x00A added a note Aug 4, 2011

so then how do you propose how to fire on an event name that has only one segment vs event names that have many segments?

@tj
tj added a note Aug 4, 2011

not sure but I think the intent for *would be everything, otherwise you would need *.*.* and *.*.*.*.* etc depending on the situation haha

@0x00A
0x00A added a note Aug 4, 2011

hrm.. i think this strengthens the need/clarity for onAny. The event name *.*.*.*.*.* etc. would simply be duplicative of onAny. I would consider *.*.*.*.*.*.foo similar in poor design to a deeply nested series of callbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tj tj and 1 other commented on an outdated diff Aug 4, 2011
doc/api/events.markdown
- server.once('connection', function (stream) {
- console.log('Ah, we have our first user!');
+
+ server.once('connection', function (value) {
+ console.log('Ah, we have our first value!');
+ });
+
+
+#### emitter.many(event, timesToListen, listener)
+
+Adds a listener that will execute **n times** for the event before being removed. The listener is invoked only the first time the event is fired, after which it is removed.
+
+
+ server.many('connection', 4, function (value) {
@tj
tj added a note Aug 4, 2011

I dont know how "cute" anyone wants this sort of thing but you could potentially do:

 server.on('connection', callback).max(4)
@0x00A
0x00A added a note Aug 4, 2011

I believe that would bring into question the existence of once.

@tj
tj added a note Aug 4, 2011

true, though once could just be the same as .max(1) as a bit of extra sugar. just throwing ideas out there while we're at it :D

@0x00A
0x00A added a note Aug 4, 2011

I actually like it quite a bit. Im just trying to be conservative.

@tj
tj added a note Aug 4, 2011

yeah it's not very node core-like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tj tj and 1 other commented on an outdated diff Aug 4, 2011
doc/api/events.markdown
console.log(util.inspect(server.listeners('connection')); // [ [Function] ]
+
+#### emitter.listenersAny(event)
+
+Returns an array of listeners that are listening for any event that is specified. This array can be manipulated, e.g. to remove listeners.
+
+
+ server.onAny(function(value) {
+ console.log('someone connected!');
+ });
+
+ console.log(console.log(server.listenersAny()[0]); // [ [Function] ] // someone connected!
+
+
@tj
tj added a note Aug 4, 2011

I'd do the same as the wildcard with a callback:

 server.on('*') => [...]
@0x00A
0x00A added a note Aug 4, 2011

see my previous comment about the * idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tj tj commented on the diff Aug 4, 2011
lib/events.js
+ return tree;
+ } else {
+ for (var leaf = 0, len = tree._listeners.length; leaf < len; leaf++) {
+ handlers && handlers.push(tree._listeners[leaf]);
+ }
+ return tree;
+ }
+ }
+
+ if (type[i] === '*' || tree[type[i]]) {
+ //
+ // If the event emitted is '*' at this part
+ // or there is a concrete match at this patch
+ //
+ if (type[i] === '*') {
+ for (var branch in tree) {
@tj
tj added a note Aug 4, 2011

everyone seems pro micro optimization so maybe Object.keys() here

@0x00A
0x00A added a note Aug 4, 2011

I can try to see if there is a perf gain there. all my v8 optimizations knowledge seems upside down since the v8 rewrite. Im still studying the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
hij1nx added some commits Aug 4, 2011
hij1nx [lib] emitting error should check this._all, listenersAny() implement…
…ed, 'un' changed to 'off' for API clarity, [doc/api] correction L#29, correction L#140, correction L#87, correction L#61, lines should not exceed 80 chars.
350520e
hij1nx [doc] slight restructure 23db018
hij1nx Merge https://github.com/joyent/node 1cfd6c6
@0x00A

closing to apply changes from @koichik's code review.

@0x00A 0x00A closed this Aug 4, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment