-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Throttle mousemove internally #2337
Conversation
Are there any cases where a user would want unthrottled events? Does this solve the problems or just make them less severe? Could the work triggered by the events still cause backups? Would it be better to throttle the actual work instead of the events that trigger the work? (should If we throttle, we should throttle with
|
ada01ca
to
4eef09d
Compare
I can't think of any, but let's see if anyone can.
It's a part of the solution, which is "avoiding unnecessary work". It can't solve all the problems but it always improves performance in case expensive mousemove handlers.
I don't think so, because throttling the actual work can have unexpected side effects and introduce subtle bugs. But specifically in case of mouse events, I can't think of a case where it would cause problems.
Agreed. Should we modify |
This looks great! 👍 to a separate |
Won't this break |
Yes, and I'm 👎 here. We've seen several instances where internal throttling in Leaflet caused issues and had to remove it. It just generally seems to be a bad idea to do it at the library level. |
But you're strongly against exposing util methods like |
Ref #2334. I’ve lost count of times where I gave advice to throttle
mousemove
whenever someone had performance issues with interactivity. I thinkmousemove
should be throttled internally by default — it always helps with performance, and I can’t think of any drawbacks/gotchas in this particular throttling case.This, coupled with the preventDefault fix in the example, makes the point dragging example much smoother. After we fix
setPaintProperty
triggeringcascade
, it’ll become butter-smooth.cc @lucaswoj @jfirebaugh @ansis @tristen