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

Decouple initial re-render onComponentDidMount delay from refreshRate #4

Closed
larrydahooster opened this issue Apr 15, 2016 · 7 comments

Comments

@larrydahooster
Copy link

Hey it is me again with a feature request/proposal. I was positively surprised that react-sizeme comes with a refresh rate, as I need to debounce the re-rendering cause of heavy computations caused by some highcharts I use.

So I simply set the refreshRate to 300ms and it worked quite perfect for me. But than I figured out, that the first re-rendering after componentDidMount, which replaces the placeholder with the real content, took also 300ms to show up.

So it would be cool to decouple the Initial re-rendering from the refreshRate, to instantly show up the content when the componentDidMount().

What do you think. I would create a PR for that.

@ctrlplusb
Copy link
Owner

Not you again! :)

Will certainly accept a PR for this and add you to the contributors list.

In regards to this issue I actually did think I had put something in place to do exactly what you are after, but looking at my implementation it is completely wrong. I used to use a streaming library for this when it was attached to a larger project. It made this type of thing really easy to manage, but it doesn't make sense bulking up this library with a large dependency just for this.

So, the code that is dodgy is the following:

line 173 of SizeMe.js

      checkIfSizeChanged = debounce(throttle((el) => {
        const { width, height } = el.getBoundingClientRect();
        const next = { width, height };

        if (this.hasSizeChanged(this.state, next)) {
          this.setState(next);
        }
      }, refreshRate), refreshRate)

I wrap the function with a throttle and a debounce from lodash, but this is completely wrong. We should have these contained within the function being executed separately whilst ensuring that we don't get refresh conflicts. Thinking something like throttle for a long period, and debounce for our configured refresh rate, but it needs some thought.

Appreciate your help and interest in this library!

@larrydahooster
Copy link
Author

Hey thanks for your reply. I think I am gonna look at it starting of next week. Have a nice weekend.

@ctrlplusb
Copy link
Owner

@larrydahooster are you still up for this? I've actually thought up a solution. I'm in need of this myself now so if you are stacked up I can pick this up. Thanks

@larrydahooster
Copy link
Author

Hey, not yet started working on that. So if you have any idea, just do it :)

@ctrlplusb
Copy link
Owner

Doh! Implemented a complex handler. Then read the lodash docs properly for throttle. It does leading and tail execution. Thats all we need. Removed the debounce and it works. 😊

@larrydahooster
Copy link
Author

Hey thanks for your changes. I always was wondering why you combined debounce with throttle. Nice that you changed it to throttle. I always was used to debounce, but throttle seams to be a nice alternative. I will stick to this right now and see later on, if I need to switch to debounce to save some performance. :) Thanks for your efforts!

@ctrlplusb
Copy link
Owner

ctrlplusb commented Apr 19, 2016

No problem, yeah I think it was late at night when I decided to combine the two!

Happy that I have at least put throttle on your radar. It's great to have the option of them.

lodash is too awesome. :)

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

2 participants