559169 - Implement a collection module that can be used to track Jetpack event handlers

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch

The high-level Jetpack APIs provide access to event handlers and things like event handlers in a unified way, like:

  obj.handlers.add(val);
  obj.handlers.add([val1, val2]);
  obj.handlers.remove(val);
  obj.handlers.remove([val1, val2]);
  obj.handlers = val;
  obj.handlers = [val1, val2];

It would be useful to have a module that defined both the |handlers| class and took care of the setter magic.

This patch adds a jetpack-core/collection modules that exports a Collections class and a method called addCollectionProperty.  Consumers can call that method to add a |handlers|-type property on an object.

Comment 1

[Drew Willcoxon :adw]

Attachment: patch with docs

Now with docs.  Should this be in some helper package instead of jetpack-core?  Is Collection an OK name?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 439350 [details] [diff] [review]
patch with docs

>+The `collection` module provides a simple array-like list class and utilities
>+for using it.

I would describe (and implement) this as a set-like class whose values must be unique, since that is the conventional behavior of web event handler interfaces like the properties for which this class is primarily going to be used (cf. addEventListener <http://www.w3.org/TR/DOM-Level-3-Events/>).

The only problem with describing this class as set-like is that sets are typically unordered, whereas this class's elements are iterated in the order in which they were added (which, incidentally, is consistent with the specification for addEventListener, and thus seems like the right behavior).


>+Adds a collection property to the given object.  Setting the property to a
>+single value adds that value to the collection, and setting it to an array adds
>+all the items in the array.

This should note that setting the property removes existing values in the process.  This might be intuitive, giving that you're setting a property, and doing that conventionally blows away its old value, but language like "adds that value to the collection" seems to imply preservation of existing values, so it would be good to be explicit here.


>+  obj.__defineSetter__(propName, function (itemOrItems) {
>+    array.splice(0, array.length);

Note that it's possible to leave out array.length from this splice call, albeit only on Spidermonkey.  I'm not sure whether it's any faster, though, and in this case speed probably doesn't matter, so we might as well leave it in on the off chance that makes the module reusable in other CommonJS environments (a lower priority than an optimal implementation, but still a nice-to-have).


>+  this.add = function Collection_add(itemOrItems) {
>+    let items = toArray(itemOrItems);
>+    for (let i = 0; i < items.length; i++)
>+      array.push(items[i]);
>+    return this;
>+  };

This should suppress duplicates by checking that |array.indexOf(items[i]) != -1| before pushing the item onto the array.


>Now with docs.  Should this be in some helper package instead of jetpack-core? 

Perhaps, but jetpack-core is fine for now.


>Is Collection an OK name?

Collection is a fine name.  It's a bit generic, but then Set is a bit inaccurate, and I don't have a better idea.  (EventSet? Ugh.)


r=myk with duplicate suppression!

Comment 3

[Drew Willcoxon :adw]

Attachment: patch v2

Thanks Myk, good points.

(In reply to comment #2)
> I would describe (and implement) this as a set-like class whose values must be
> unique, since that is the conventional behavior of web event handler interfaces

Went with:

"The collection module provides a simple list-like class and utilities for using it. A collection is ordered, like an array, but its items are unique, like a set."

> This should note that setting the property removes existing values in the
> process.

"Setting the property to a scalar value empties the collection and adds the value. Setting it to an array empties the collection and adds all the items in the array."

I also refactored the test a little, added checks for uniqueness, and clarified some other parts of the docs.

Comment 4

[Drew Willcoxon :adw]

http://hg.mozilla.org/labs/jetpack-sdk/rev/2d04e34a547c

Comment 5

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".