Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Zegnat opened this issue Oct 27, 2014 · 5 comments

Comments

@Zegnat
Copy link

Zegnat commented Oct 27, 2014

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
Copy link

lazd commented Nov 5, 2014

+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.

wesleytodd added a commit to wesleytodd/gator that referenced this issue Nov 5, 2014
wesleytodd added a commit to wesleytodd/gator that referenced this issue Nov 5, 2014
wesleytodd added a commit to wesleytodd/gator that referenced this issue Nov 6, 2014
@lazd
Copy link

lazd commented Nov 6, 2014

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 added a commit to wesleytodd/gator that referenced this issue Nov 6, 2014
@wesleytodd
Copy link
Contributor

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
Copy link
Author

Zegnat commented Nov 7, 2014

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
Copy link

lazd commented Nov 7, 2014

@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:

  • When on(event) is called
    • parse event to extract a list of namespaces as namespaces_in
    • store the list as listener.namespaces = namespaces_in
  • When off(event) is called
    • parse event to extract a list of namespaces as namespaces_in
    • for listeners as listener
      • if listener.namespaces contains every entry in namespaces_in
        • remove listener

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants