erd.detach(element) to stop listening on an element

[m59peacemaker]

I've implemented erd.detach(element) with a test for the object strategy and scroll strategy. Because the IE8 object strategy applies the events directly to the element and I'm not sure the best way to test my work on IE8, detach isn't supported on IE8 for now.

[wnr]

Hi, and thanks for the PR.

I've had a look at the code and it seems solid. Before merge, there are a few things I would like to discuss.

I can identify three use cases for the detach functionality:

1. To detach a specific resize listener callback for an element.
2. To detach all resize listener callbacks for an element.
3. To completely uninstall the resize detector for an element, in order to free memory (or any other reason).

It should be noted that case 1 and 2 should not uninstall the resize detector, since new listeners might be attached to an element in the future (and we want to avoid the installation cost).

Therefore, I propose the following additions to the erd API:

• erd.stopListening(element, [callback]) to take care of use case 1 and 2. If callback is present, only the specified callback is detached, otherwise all callbacks are detached for the element.
• erd.uninstall(element) to take care of use case 3.

I've not used the detach method name since I think it is ambiguous whether it means to remove a listener callback or uninstalling the resize detector. Also, listenTo seems to go hand-in-hand with removeListener in other API's (mainly Backbone). This could perhaps be renamed to addListener/removeListener or something similar in the future.

What do you think?

[m59peacemaker]

I think one of the cases is an over-optimization. I don't think it would be common that someone would need to cancel all listeners and add more later, but virtually unthinkable that they would need to do so in a context where the performance of their app would depend on that optimization. I suggest keeping the code lighter and leaving that possibility up to a future pull request if it is ever needed.

[m59peacemaker]

Well, now that I said all that... I'm looking at the code and it kind of makes sense to go ahead and do it the way you suggested.

[wnr]

Great!

Do you want to do it, or shall I do it? :)

[m59peacemaker]

I'm almost done :) Is my other pull request ok? I'm using it in this feature, but I can change it if I need to. It just makes it easy to clean up everything for the uninstall with cleanState(element) instead of having to remove a bunch of properties.

[wnr]

Oh okay, cool. No stress, just wanted to know if you were on it :)

Your other PR has now been merged to branch 0.4.0 (which I'm looking to merge this PR into as well). I also changed a few things in the code, I hope this is not too inconvenient for you.

[m59peacemaker]

The uninstall tests probably need some work.. they don't really distinguish from removeAllListeners. I'm not sure the best way to approach that. I'm a bit inexperienced with teamwork on GitHub... I pushed my commit that has my other PR in it already. It's all confusing to me at this point. I will be able to look at this all again later tonight and try to figure it out and also update the README.

[wnr]

The uninstall tests probably need some work.. they don't really distinguish from removeAllListeners. I'm not sure the best way to approach that.

I see. I'll have a look at it as well perhaps tomorrow.

I'm a bit inexperienced with teamwork on GitHub... I pushed my commit that has my other PR in it already. It's all confusing to me at this point.

That's okay :) I suggest you merging the 0.4.0 branch into this branch, which should not be too hard. This would make it easier for me to review, and then we don't have diverged branches anymore.

I will be able to look at this all again later tonight and try to figure it out and also update the README.

Sounds good, no stress.

[m59peacemaker]

I had time to go ahead and make those improvements. I think the new tests using the childNode length is a good approach, since we know the test element doesn't have any other children.

[wnr]

Very nice, this has now been merged to the 0.4.0 branch (not yet released on npm).

Before release I would like to clean up the tests a bit (as you suggest). I agree with fixtures being a bad idea, and would rather have each it statement own it's own DOM data (to be added and removed for each it).

Do you want to do this? :)

If yes, I suggest creating a new PR against the 0.4.0 branch.

Cheers