search

ccampbell / gator

Event delegation in Javascript
http://craig.is/riding/gators
492 stars 46 forks source link

[Documentation] Using the `new` keyword is not synonymous to calling the library without it. #14

Open Zegnat opened 10 years ago

Zegnat commented 10 years ago

Currently the full documentation on create a gator instance reads:

The methods that do event delegation are prototype methods on the Gator object. To instantiate a Gator object call the function directly:

Gator(element);

or use the new keyword

new Gator(element);

This is wrong as those two calls are not synonymous. In fact, using the new keyword will break code. Bug reproduction:

<!doctype html>
<button>Ride a Gator</button>
<script src="gator.js"></script>
<script>
new Gator(document.body).on('click', 'button', function(){
    alert('Saddle pain sucks.');
});
</script>

Clicking the button will throw a TypeError.

Please remove the mention of the new keyword method from the documentation.

lazd commented 10 years ago

+1, Gator is a factory, not a constructor.

After removing support for new, the Gator variable should be changed to gator so folks don't think it's a constructor.

lazd commented 9 years ago

In looking at this issue and @wesleytodd's PR #14 and working on another event delegation library, I realized there is actually a benefit to having multiple Gator instances per element... You can use it as a kind of "event namespacing," where events on a certain "namespace" are added to one instance can be removed wholesale if necessary, without affecting events added by other Gator instances...

Currently, it's not possible to create more than one Gator instance per element despite there being a valid use case.

However, if the behavior of new was modified as such, it would be possible:

  1. When Gator(element) is called for the first time for a given element, create a "singleton" Gator instance for that element and return it

    1.1. If Gator(element) called again, return the "singleton" instance

  2. When new Gator(element) called, return a new instance for that element

    2.1. If Gator(element) is subsequently, create or return the "singleton" instance as defined in 1

That is, instances created with new would not be returned by the factory for subsequent calls, whereas instances created without new would be reused.

With this proposal, the following would be possible:

// A listener added elsewhere for clicks on the body
Gator(document.body).on('click', someoneElsesHandleClick);

// ...

// An application that uses events added with Gator
var myApp = { /* ... */ };

// A Gator instance for listeners related *only to myApp*
myApp.gator = new Gator(document.body);

myApp.gator.on('click', '.js-reset', myApp.reset);
myApp.gator.on('submit', '.js-signup', myApp.signup);

/// ...

// Remove all listeners related to myApp
// This should leave the listeners added on Gator instances other than myApp.gator
myApp.gator.off();
wesleytodd commented 9 years ago

Personally I would rather have the straight forward behavior of of the constructor. Then have explicit namespaces for events, something like: namespace:click or namespace.click. People are probably already used to that kind of namespacing as opposed to this more obscure method.

But really I would rather that be a plugin or extension, not a part of the main library. The main reason I chose this library over others is that it only solves this one simple problem that I have...delegation.

Zegnat commented 9 years ago

So in terms of Gator, @lazd is proposing a separate _gatorInstances array for every new Gator instance?

I think I personally think that Gator does very well being a factory (much like your first comment) and agree with @wesleytodd that the current ‘straight forward behaviour’ is probably exactly what people want. Especially those who – like me – find it via Microjs and only want it to handle events.

Namespacing might be interesting to support. Is there some sort of consensus among libraries on how to namespace events? Quick Googling suggests only jQuery supports it and uses the format event.namespace.

Edit: Google/JsAction actually seems to support it too, as event:namespace, but this seems to have a different use and I don't think it can be used for .off()ing them all at once. Gator seems much closer to jQuery so sticking to event.namespace is probably preferred.

lazd commented 9 years ago

@Zegnat, not quite, see this fiddle for a simple constructor that implements the pattern I'm describing, let's call it the Memoized Singleton with Override. Basically, the input is "memoized" and you get the same instance back for the same input, unless you call the constructor with new, in which case input is not memoized and you are always guaranteed to get a new instance that others cannot get by passing the same input as you.

This would allow both the factory pattern Gator(element) and the constructor pattern new Gator(element), and would enable folks to create a fresh Gator instance for each "namespace" of events that they can remove with a simple instance.off().

Since new is completely broken as is, it's actually a backwards compatible change.

That said, I do feel that event namespaces are a more straight-forward pattern and most people will find familiar, but it does go against the "micro" nature of this library. The solution outlined above can help accomplish the same basic goal as namespacing -- give the user an easy way to remove a set of events all at once -- without the added complexity, but it is less flexible than namespaces because a listener can only be part of one instance, whereas, with namespaces, a listener can be part of multiple namespaces and will be removed when off() is called with any namespace it is a part of.

As far as the convention for namespaces, this is the pattern I've observed:

With the above algorithm, the implementation will behave as such:

off('.ns1.ns2') // only remove listeners that has both ns1 and ns2 namespaces
off('.ns1') // remove any listener that has the ns2 namespace
off() // remove all listeners