Making callbacks consistent

[padolsey]

These commits fix the callback ambiguity we had across the project. All callbacks are now using the single-callback pattern used in node.js:

foo(function(err, data) {
  if (err) // error
  else // success
});

In addition the argument patterns used in Movie, Video, Bitmap and FontFamily have now been aligned. A convenience callback argument can be used (with the above pattern) or you can simply bind to the load and success events:

new Bitmap(url).on('load', function(){ ... });

Before, this might not have worked if the bitmap loaded immediately from cache. The load event would be fired before instantiation completed, meaning that listening for it after that would be pointless.

But now, using a new asyncEmit method on the EventEmitter, we now trigger these events asynchronously. I'm not sure if we want there to be a method especially for this or if we should inline the setTimeout in relevant places. Up for discussion on that.

[klipstein]

Should be removed as it is not part of the repo.

[padolsey]

Ok. It's been removed.

[davidaurelio]

I like the branch, good work!

A few remarks:

• this is nitpicking, but to me emitAsync sounds more like a method name than asyncEmit. No need to change, if you like your version better.
• I’m not convinced of the callback-to-event-listener conversion: it shadows what’s happening behind the scenes (is it a one-time callback? is it a general listener?). It also requires redundant code. I would wipe that completely and go with events exclusively. But that’s a matter of taste, no idea what others are thinking. I generally prefer solutions where there’s only one obvious way to do something.

[padolsey]

I will change the method name to emitAsync. I guess it does sound more like a verb.

I still need to think a bit about the events-only approach. It's very rare that you'd want to instantiate a Bitmap, for example, without listening for its load event, so it seems sensible to minimise the complexity and the amount of code it takes to do that:

new Bitmap(url, function(){});
// vs.
new Bitmap(url).on('load', function(){});

The events-only approach seems more generalised. The former approach, with a callback passed to the constructor, is more suited to the purpose of making a new Bitmap and waiting for it to load. It depends how generalised we want to be with our API.

For me, I like knowing about and using API shortcuts. And it's always cooler to use an API when there are multiple ways to bend it to your will. Maybe this is an emotional appeal. I just think we should imagine what the user will want to do and then mould the API to best suit those imagined intentions...

[padolsey]

I guess we can put the events-only thing to a vote in the flow?

[davidaurelio]

Yes, let’s invite everybody here to comment on that.

[padolsey]

To be clear for people arriving here, the two options are a more generalised approach:

new Bitmap(url).on('load', function(){...});

Or a "convenience" callback as the second arg:

new Bitmap(url, function(){...});

And with the second approach you're still free to do .on() calls. It's just convenience.

Whatever is decided will also count for Video, FontFamily and Movie.

[klipstein]

I would prefer to have both, so I'm +1 on the "convenience" callback.

[davidaurelio]

• Is it possible to unregister the convenience callback?
• Shouldn’t the convenience callback be added with once()?
• I feel that the example chaining on() calls is much clearer to read.

[klipstein]

• it does not need to be unregistered because it never was registered
• you should be able to do both. There are cases where the callback-style can be written in one line: new Bitmap(url, function(){ stage.add(this); }); and if there isn't a convenience callback-styled approach I'm always forced to write a second line.
• I think both styles can be read well and it depends on the use-case. It is more a matter of taste in this case. The convenience method should be there for the default use-case.

[padolsey]

Yes, definitely, the convenience callbacks should be bound with once instead of on (@tobias: behind the scenes, we do bind to the events for the regular callback). I'll add that if we go down that route.

I feel that the example chaining on() calls is much clearer to read.

I actually agree, but I still think it's a good idea to offer convenient args for popular use-cases.

[klipstein]

Is it ensured that this callback always will be called? We need to be careful in this case to avoid memory leaks.

[davidaurelio]

• it does not need to be unregistered because it never was registered

Like James said: it is registered, you just don’t know. Even if registered with once(), the semantics don’t match the expectations of a callback:

function Thing(callback) {
  this
    .once('load', function(foo, bar) {
      callback.call(this, null, foo, bar);
    })
    .once('error', function(error) {
      callback.call(this, error);
    });
}

when load has been fired, the error handler is never unregistered (i.e.: will fire for any future error event). Unregistering adds a level of complexity to our code that is not justificable in from point of view. Also: tests for this behavior are needed.

• you should be able to do both. There are cases where the callback-style can be written in one line: new Bitmap(url, function(){ stage.add(this); }); and if there isn't a convenience callback-styled approach I'm always forced to write a second line.

I can write that without a second line:

new Bitmap(url).on('load', function(){ stage.add(this); });

• I think both styles can be read well and it depends on the use-case. It is more a matter of taste in this case. The convenience method should be there for the default use-case.

Evidently, even for us it’s impossible to predict the exact behavior of the callback. Although we know the implementation. It just has side effects we are not completely aware of.

[klipstein]

Didn't know that it is using once internally. If we don't provide a safe way to cleanup the listeners I then would say: we shouldn't do it.

@davidaurelio you are right about writing it in one line if you don't do error-handling. In the case of an error you would have to maintain two callbacks.

What happens if you do the following:

new Bitmap(url).on('load', function(){ stage.add(this); }).on('error', function() {});

What happens if the 'load' event is triggered? Does it cleanup the error callback for me or do I have to do it on my own?

[padolsey]

@klipstein, in that case you'd have to clean it up yourself.

@davidaurelio I see that to do it properly we'd need additional code, so I guess I now agree that we should remove the convenience callbacks.

[davidaurelio]

@padolsey Reading your hint that the user would need to cleanup an error handler on his own I can see a use case for the callback :) Maybe we should do it properly and put the code in a tools method.

[padolsey]

@davidaurelio, Or we let Movie, Video, FontFamily and Bitmap extend a new AssetDisplayObject class which takes care of this for us?

[padolsey]

I see this load/error callback thing as being unique to these specific DIsplayObjects so I think it'd make sense to encapsulate the logic in an interface or whatever... instead of placing it in tools.

[davidaurelio]

ok, sounds good.

[padolsey]

@davidaurelio I've added a asset_display_object mixin which takes care of the callback binding/unbinding. Please review.

[davidaurelio]

Ok, just reviewed it and I LIKE. Could you please add a line to the changelog? Thank you. Will merge afterwards

[padolsey]

Ok, just modified the changelog. Linked to this pull request too -- hope that's appropriate.

[davidaurelio]

That’s cool, thank you!