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

No throttling on resize event handlers. #1990

Closed
goulashify opened this issue Oct 6, 2017 · 9 comments
Closed

No throttling on resize event handlers. #1990

goulashify opened this issue Oct 6, 2017 · 9 comments

Comments

@goulashify
Copy link

goulashify commented Oct 6, 2017

Issue

When you resize windows fast, everything is laggy, and you feel like:
image


My take on this:

Cause

The resize event handler is not throttled.

Solution

import { throttle } from 'lodash'; // You have it as dependency already.

  {/* ... component stuff */}
   componentDidMount() {
    window.addEventListener('resize', this.onResize);
  }

  componentWillUnmount() {
    window.removeEventListener('resize', this.onResize);
  }

  onResize = throttle(this.handleOnResize, 200, { trailing: true });

  {/* onResize renamed to handleOnResize */}
  {/* ... more component stuff */}

Apart from that

Thank you for making this!

<3
-Dan

@goulashify goulashify changed the title Add throttling to the resize event handlers. No throttling on resize event handlers. Oct 6, 2017
@shilman
Copy link
Member

shilman commented Oct 6, 2017

@danigulyas care to issue a PR for this? it looks like a very reasonable solution, though I haven't run into the problem myself AFAIK so it's hard for me to tell!

@Hypnosphi
Copy link
Member

I'd prefer to use requestAnimationFrame throttling rather than timer-based one

@goulashify
Copy link
Author

@shilman sure thing, somewhere on the weekend.
@Hypnosphi good idea, will check up on that!

@goulashify
Copy link
Author

goulashify commented Oct 14, 2017

So i dug into this more, did a little timeline action, turns out the resize is kind of slow in general, i've tested the same html inside and outside the context of Storybook, it was similarly slow, which is in a way understandable for a while (this browser stuff is still good for free!).

However, after taking a second look on onResize, it seems to me that it couldn't even react to events when passed in like this (its just a higher order function, should've seen that before opening the issue).

See, onResize is registered as an event handler here, but it is actually being used for something different and it returns a function which is ought to handle some event with size as a parameter.

@Hypnosphi: I couldn't find requestAnimationFrame or something similar in the lib which the sidebar is implemented with, but that's not really a performance bottleneck in my opinion (you can check the timeline of rerenders with rect_perf and decide for yourself).

@shilman: So yup, this issue i guess is solved, given that it didn't exist to begin with, what do you think?

@wuweiweiwu
Copy link
Member

wuweiweiwu commented Mar 9, 2018

@shilman @Hypnosphi @danielduan I just took a peek at the code and im 99.99% sure the issue lies because we dispatch setState too many times here

And MDN even has a page dedicated to how to throttle resize events.

Also im 99.99% sure this code is not resizing, adding onResize as an event listener doesnt do anything. Every thing is done in the onChange handler that calls onResize's return function

  componentDidMount() {
    window.addEventListener('resize', this.onResize);
  }

  componentWillUnmount() {
    window.removeEventListener('resize', this.onResize);
  }

But I'll have a PR out for this in the next couple of days when I have time :)

@Hypnosphi Hypnosphi assigned Hypnosphi and unassigned Hypnosphi Mar 9, 2018
@danielduan
Copy link
Member

In that case, a throttle probably makes more sense than requestAnimationFrame?

we shouldn't be running setState 60x / sec even if the computer is fast enough to support it. Looking forward to your PR, thanks @wuweiweiwu

@Hypnosphi
Copy link
Member

Hypnosphi commented Mar 9, 2018

we shouldn't be running setState 60x / sec

Why? I'd like the resizing to be smooth

UPD: OK, it's not about resizing itself, it's about updating dimension numbers. Then throttling on ~200 ms sounds reasonable

@wuweiweiwu
Copy link
Member

@Hypnosphi @danielduan Done! I took a slightly different approach and delayed state update until the user stops dragging for 50ms using setTimeout and clearTimeout so we don't call setState a billion times :)

@Hypnosphi
Copy link
Member

Released as 3.4.0-rc.1

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

No branches or pull requests

5 participants