Closed Bug 910345 Opened 8 years ago Closed 8 years ago

[Clock] Create Emitter abstract for events

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gnarf, Assigned: gnarf)

References

Details

Attachments

(1 file, 2 obsolete files)

We want to have a reusable Emitter that we can use for events inside the new clock APIs.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #796789 - Flags: review?(mike)
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.
Attached patch patch v2 (obsolete) — Splinter Review
adresses rick's comments
Attachment #796789 - Attachment is obsolete: true
Attachment #796789 - Flags: review?(mike)
Attachment #796802 - Flags: review?(mike)
Comment on attachment 796802 [details] [diff] [review]
patch v2

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

Great work!
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.
Attachment #796802 - Flags: review?(mike)
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?
Attached patch patch v3Splinter Review
Adds .once(type, handler) to bind an event only called once.

Adds jsdoc style commenting to hopefully help with documentation
Attachment #796802 - Attachment is obsolete: true
Attachment #797316 - Flags: review?(mike)
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."
Attachment #797316 - Flags: review?(mike) → review+
Comment on attachment 797316 [details] [diff] [review]
patch v3

Also: nice work!
master: https://github.com/mozilla-b2g/gaia/commit/cfb1a7d3a24f4d0d7c1730ed94dff2b85a124dfc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.