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

Deprecate mouseEnter/leave support for Ember.Component event handler methods? #474

Closed
simonihmig opened this issue Apr 7, 2019 · 5 comments

Comments

@simonihmig
Copy link
Contributor

simonihmig commented Apr 7, 2019

Thinking about whether we should deprecate support for mouseEnter/Leave events as event handler methods in Ember.Component (https://guides.emberjs.com/release/components/handling-events/#toc_event-names).

There is a quite high overhead to support them: the EventDispatcher's way of handling events through event delegation requires quite some awkward shenanigans to support these events, as they do not bubble, so simple event delegation just does not works (see emberjs/ember.js#16591). jQuery has some custom support by actually listening for (bubbling) mouseover/out events and faking mouseenter/leave.

In no jQuery-mode I added a similar approach here: emberjs/ember.js#16603, but this has quite some performance-related drawbacks (concerns were already raised at emberjs/ember.js#16591 (comment)):
it (optimistically) creates fake mouseenter events and tries to dispatch them to components having a mouseEnter(mouseLeave) event handler method. But in like ~99,99% (guessing) of the cases there won't be a component really listening for those events, which nevertheless fire in a very high frequency when the mouse is moving around, causing unnecessary JS execution, GC etc.

I guess this could be better optimized (i.e. creating the fake event only when a component requiring it has been found), but nevertheless I think there is little value in that support, as it can be implemented easily using custom listeners / closure actions / {{action}}/{{on}} modifiers. Especially as with Glimmer components we move toward the latter anyway.

I think back then we had no choice than providing support, as both support for these events as well having jQuery-less apps were public APIs we had to support, but I feel we should deprecate that now, and drop support in 4.0.

Thoughts? I can write a proper RFC if it makes sense...

/cc @rwjblue

@rwjblue
Copy link
Member

rwjblue commented Apr 12, 2019

I am absolutely in favor of this!

@rwjblue
Copy link
Member

rwjblue commented Apr 12, 2019

FWIW, I just implemented the ability for the event listeners to be setup lazily (and therefore avoid setting up listeners for things that are unused) over in emberjs/ember.js#17911. This should help avoid things like the passive warning on chrome 51+ and simply not setup mousemove / mouseenter listeners at all (since IMHO most apps don't use them).

A bit more work is needed to confirm that PR doesn't accidentally make other things slower than intended though.

@simonihmig
Copy link
Contributor Author

@rwjblue Just to make sure we are on the same page: even with the lazy support being available, deprecating these events still make sense, as the lazy support does not introduce something like teardownHandler() once no component/action handler is not listening to a given event anymore. So once the handler has been set up, it stays around. So writing up an RFC still makes sense, agree?

What do you think about mousemove btw? It does not have the higher implementation cost of mouseenter, but it still has so much traffic and is rarely used IMHO.

@rwjblue
Copy link
Member

rwjblue commented Apr 12, 2019

Absolutely! I was only mentioning it because I think the efforts are complimentary in general, and this issue made me think of that solution.

@simonihmig
Copy link
Contributor Author

Closed by #486

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

2 participants