tropo / PhonoSDK

Phono.com - The jQuery Phone API
http://phono.com
Other
131 stars 63 forks source link

EventEmitter style handlers #30

Open loopingrage opened 11 years ago

loopingrage commented 11 years ago

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:

$.phono({
  apiKey: 'foobar',
  onReady: function() {
    $("#call").attr("disabled", false).val("Call");
  }
});

could also be written as:

var phono = new Phono({
  apiKey: 'foobar'
});

phono.on('ready', function() {
    $("#call").attr("disabled", false).val("Call");
});
loopingrage commented 11 years ago

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);
});
fairchild commented 11 years ago

I think this looks good.

HenrikJoreteg commented 11 years ago

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:

  1. EventEmitter2: https://github.com/hij1nx/EventEmitter2 - more feature complete, arguably a bit over the top.
  2. Emitter: https://github.com/component/emitter - purposely basic, doesn't support wildcards.

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.

fairchild commented 11 years ago

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 did you have in mind something like how jquery is doing it, http://api.jquery.com/on/? I think it would be very similar, but wondering what the equivalent of the event object passed to the hander would be in this case.

loopingrage commented 11 years ago

i'm working this on the flight home from paris today. will push to a branch sometime tomorrow with some examples.

HenrikJoreteg commented 11 years ago

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
});
loopingrage commented 11 years ago

Pushed just now to the 'on-handlers' branch: https://github.com/phono/PhonoSDK/commit/0f61510e48693d6f2f53dfd0ca2348ee5d8f5156

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 ant from modules/phono-js. This will create a new jquery.phono.js in the target directory.

Verified that the following now works (note that you need to test from a webserver as WebRTC does not work from a file:// URL):

<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>
fairchild commented 11 years ago

Tried this out, and it seems to work pretty good. im assuming you would add phono.on('message') and phono.on('incomingCall')

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 phono.messaging.send I am getting Uncaught ReferenceError: $msg is not defined

fairchild commented 10 years ago

@loopingrage has this been merged into the latest release?

fairchild commented 10 years ago

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: