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

Namespaces, unbinding and the .on() syntax #55

Open
louisameline opened this issue May 22, 2016 · 7 comments
Open

Namespaces, unbinding and the .on() syntax #55

louisameline opened this issue May 22, 2016 · 7 comments
Milestone

Comments

@louisameline
Copy link

louisameline commented May 22, 2016

Hi,

it would be nice to manipulate hoverIntent with jQuery's .on syntax which offers namespacing and easy unbinding with .off(). When there would be no listeners left, the bindings should be automatically removed to prevent memory leaks (no need for the manual destruction suggested in #43).

$(selector).on('hoverIntent.namespace' [, selector] , handlerIn, handlerOut);
// or
$(selector).on('hoverIntent.namespace' [, selector] , handlerInOut);
// and then
$(selector).off('hoverIntent.namespace');
// or 
$(selector).off('.namespace');

It should be possible to hook on jQuery's event system to make it happen. Thank you

@dim2k2006
Copy link

Is there any information about this issue? Was this functionality implemented?

@usmonster usmonster modified the milestone: v1.9.0 Feb 23, 2017
@usmonster
Copy link
Collaborator

Hi @louisameline and @dim2k2006! Thanks for the ping on this. This might be a good idea, though can you please let me know if unbinding events via $(selector).off('.hoverIntent') would be a sufficient solution for your use case?

I'd just like some more discussion before deciding whether or not this is a common enough use case to include it in the upcoming milestone. :)

@louisameline
Copy link
Author

louisameline commented Feb 23, 2017

Hello, no, I need to apply my own namespaces. It feels wrong that you ask me to use your namespace anyway, it's precisely done for the user to customize his stuff. Jquery's event system is built exactly to handle libraries like hoverintent and you're missing features by not using it. I can't feel comfortable with this library if you don't handle events the way jquery does, this is a jquery plugin after all. And I really mean no offense but as a developer, I thought I'd let you know that the fact that this library misses that point doesn't make me confident about the author's skills and doesn't make me want to use it. I'm not saying that's a fact, I mean it's just the impression it gives. But that might just me. Thanks anyway for what you did so far.

@briancherne
Copy link
Owner

Hi @louisameline! I'm sorry, but I actually took offense... which is usually what happens when you ask someone not to ;)

You should be confident in the author's and maintainer's skills, and the quality of the code, when a plugin (never part of the core library) is still compatible and actively in-use 10 YEARS after it was created... and the only open issues are of the feature-request variety.

We've made changes to modernize hoverIntent before. Per this issue (#55) I'm not opposed to extending how hoverIntent is applied to elements so long as it can be done without breaking existing implementations. Someone just needs to write the pull request.

@louisameline
Copy link
Author

Hello, I apologize for hurting your feelings.

It's a good thing that this library works after 10 years, kudos for that :) What I tried to say, with the wrong words, is that IMHO it would need a fresh paint coat to meet the API quality I expect from a modern plugin.

As for breaking existing implementations, that's what major versions are for in semver, I see no issue here. The current API is unpractical and will be redundant with jQuery's event system, so it should be removed from a v2.

As a side note, the unbinding thing is real issue though, it leads to memory leaks. I was initially discouraged to make a PR as I was not sure if this project was still maintained. It seems that a v1.9 by @usmonster has been in the works for a long time, but his fork has many branches, maybe he could clarify where we can find the latest project files? It would be nice if there was a dev branch here on this project instead.

Thanks.

@usmonster
Copy link
Collaborator

Hi @louisameline!

It seems that a v1.9 by @usmonster has been in the works for a long time, but his fork has many branches, maybe he could clarify where we can find the latest project files?

To address your question/misunderstandings:

  • v1.9 (not yet released) is the master branch of this repository. Other versions are tagged.
  • My personal fork's branches are meaningless for you, as the repo description says. ;)
  • Yes, the milestone has 3 "easy" non-dev tasks left, which haven't been done for a while. My bad! In any case, the master branch is functionally equivalent to what will eventually be v1.9 and is safe to use in production.

As for the change you propose, I agree with your reasoning to use the "jQuery way" of event handling, especially in preference over #43. Still, you shouldn't be surprised that we'd of course want to discuss your specific use case to see if there's general enough need, to make sure the problem (memory leak) exists on the current version, and only then decide if & how to prioritize it.

All that said, I think a PR would be welcome for this, if you're willing to make one. If not for v1.9, I can see this in a v2.0, if @briancherne agrees.

@louisameline
Copy link
Author

Ok, thank you for your explanations.

@usmonster usmonster modified the milestone: v2.0.0 Sep 4, 2017
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

No branches or pull requests

4 participants