-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EventEmitter style handlers #30
Comments
More complete example would be something like: var call = phone.dial('tel:3055195825');
call.on('ring', function() {
$("#status").html("Ringing");
});
call.on('answer', function() {
$("#status").html("Answered");
});
call.on('hangup', function() {
$("#call").attr("disabled", false).val("Call");
$("#status").html("Hangup");
});
call.on('error', function(err) {
console.log('Oops!', err);
}); |
I think this looks good. |
From my experience, it's also very nice to have support for more than one handler for a single event. So you can do call.on('hangup', function() {
console.log('hangup handler 1');
});
call.on('hangup', function() {
console.log('hangup handler 2');
});
call.hangup();
// triggers both handlers
// "hangup handler 1"
// "hangup handler 2" Also, I'm personally a big fan of supporting catchall callbacks that get triggered on each event. It makes it super easy to just for example log out all events, or to namespace events. For example: call.on("*", function () {});
// or
call.on("call*", function () {}); This is why I created https://github.com/HenrikJoreteg/wildemitter. Other options/reference emitter implementations:
It's also quite nice to (in the case of phono) have the main phono object emit things like "hangup" and not just the call object. That way you can register handlers for something that happens at the end of every call without having to re-register those handlers. You can just emit from the call object and from the base phono object so you can register handlers on either. It works really well. Anyway, there's my two cents. |
I think it woudl be great to see this api style added. I like the event model, and that it can be more easily extended, and chained. I think there would then need to be some standard set of arguments or options that get passed thru as function arguments. What could that look like? @HenrikJoreteg |
i'm working this on the flight home from paris today. will push to a branch sometime tomorrow with some examples. |
Regarding conventions for arguments passed to callbacks. It's a bit tricky since the types of objects you get are so dependent on the type of event. There are a few established conventions, like error first. But those make more sense in cases where you are calling code that may go wrong. For example. api.dial('909324234', function (err, call) {
if (err) {
// handle the error
} else {
// do something with the call
}
}); But when registering event handlers this make no sense. Since you're essentially specifying the type of argument you want to receive based on the function. Take for instance: api.on('incomingCall', function (call) {
// the first argument here, should definitely be the call and error wouldn't make sense
// because this callback simply wouldn't be called if there was an error.
}); It makes sense to do error callbacks, though. api.on('error', function (err) {
// determine type of error and handle it
}); |
Pushed just now to the 'on-handlers' branch: 0f61510 As expected, it was only ~8 lines of code per object since our internal event dispatcher already had the support. To try for yourself, just check out PhonoSDK and run Verified that the following now works (note that you need to test from a webserver as WebRTC does not work from a <html>
<head>
<script src="http://code.jquery.com/jquery-1.4.2.min.js"></script>
<script src="jquery.phono.js"></script>
</head>
<body>
<input id="call" type="button" disabled="true" value="Loading..." />
<span id="status"></span>
<script>
$(document).ready(function(){
var phono = $.phono({
apiKey: ""
});
phono.on('ready', function() {
$("#call").attr("disabled", false).val("Call");
});
$("#call").click(function() {
$("#call").attr("disabled", true).val("Busy");
var call = phono.phone.dial("985-655-2500");
call.on('ring', function() {
$("#status").html("Ringing");
});
call.on('answer', function() {
$("#status").html("Answered");
});
call.on('hangup', function() {
$("#call").attr("disabled", false).val("Call");
$("#status").html("Hangup");
});
});
})
</script>
</body>
</html> |
Tried this out, and it seems to work pretty good. im assuming you would add I did not get around to trying the chaining style that @HenrikJoreteg suggested, but hoping that would still work also. fyi, probably unrelated, but when trying |
@loopingrage has this been merged into the latest release? |
We have been doing some development on this event emitter style and have settled on the following basic events. Since it looks like this event emitter style has not been merged in yet, what do you think of supporting the following:
|
In addition to the jQuery-style onEvent handlers we should also support the Node.js EventEmitter style to be more comfy for noders joining the community.
For example:
could also be written as:
The text was updated successfully, but these errors were encountered: