910345 - [Clock] Create Emitter abstract for events

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Clock, defect)

Description

[Corey Frang [:gnarf]]

We want to have a reusable Emitter that we can use for events inside the new clock APIs.

Comment 1

[Corey Frang [:gnarf]]

Attachment: patch v1

Comment 2

[Rick Waldron [:rwaldron]]

Comment on attachment 796789 [details] [diff] [review]
patch v1

Review of attachment 796789 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/clock/js/emitter.js
@@ +13,5 @@
> +// Emitter.mixin(MyConstructor.prototype);
> +// -or-
> +// Emitter.mixin(MyObject);
> +function Emitter() {
> +}

Might as well make this one line, like: `function Emitter() {}`

@@ +19,5 @@
> +
> +// add our prototype properties to an arbitrary object
> +Emitter.mixin = function(target) {
> +  // generate a non-enumerable property for each method
> +  for (var method in Emitter.prototype) {

If someone else adds things to Emitter.prototype and then I use Emitter.mixin(...), that stuff will be copied as well.

Comment 3

[Corey Frang [:gnarf]]

Attachment: patch v2

adresses rick's comments

Comment 4

[Rick Waldron [:rwaldron]]

Comment on attachment 796802 [details] [diff] [review]
patch v2

Review of attachment 796802 [details] [diff] [review]:
-----------------------------------------------------------------

Great work!

Comment 5

[Mike Pennisi [:jugglinmike]]

Comment on attachment 796802 [details] [diff] [review]
patch v2

Review of attachment 796802 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work here, Corey! There are two common pub/sub use cases this emitter does not address:

1. Unbinding *all* event handlers, regardless of type. For example, Backbone.js's "Events" object supports this through calling `.off()` with no arguments, and Node.js's "Emitter" module supports this through calling `.removeAllListeners()` with no arguments.
2. Subscribing to an event for one time only. Both Backbone.js and Node.js implement a `once` method so that handlers are only triggered once.

Given that these are common use-cases in evented JavaScript code, I was wondering if you omitted them intentionally, or if you think they might make for nice additions to what you have here.

::: apps/clock/js/emitter.js
@@ +52,5 @@
> +  return this;
> +};
> +
> +Emitter.prototype.off = function(type, handler) {
> +  var events = getEvents(this, type, true);

The third argument here is unused.

Comment 6

[Mike Pennisi [:jugglinmike]]

Comment on attachment 796802 [details] [diff] [review]
patch v2

Also, since our goal is to one day move this utility to `shared/`, I think documentation is especially important. Would you mind adding JSDoc-style comments to each of the public methods?

Comment 7

[Corey Frang [:gnarf]]

Attachment: patch v3

Adds .once(type, handler) to bind an event only called once.

Adds jsdoc style commenting to hopefully help with documentation

Comment 8

[Mike Pennisi [:jugglinmike]]

Comment on attachment 797316 [details] [diff] [review]
patch v3

Review of attachment 797316 [details] [diff] [review]:
-----------------------------------------------------------------

Pending one minor addition to the documentation, this looks good to me.

::: apps/clock/js/emitter.js
@@ +83,5 @@
> +/**
> + * Unbind Event Handlers
> + * @param {string} [type] The event type to unbind.  Removes all event
> + *                        types if omitted.
> + * @param {function} [handler] The function to unbind.

Add, "Removes all handlers of the specified 'type' if omitted."

Comment 9

[Mike Pennisi [:jugglinmike]]

Comment on attachment 797316 [details] [diff] [review]
patch v3

Also: nice work!

Comment 10

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/cfb1a7d3a24f4d0d7c1730ed94dff2b85a124dfc