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

Investigate rAF scheduling - seems unnecessary #216

Closed
Andarist opened this issue Jul 21, 2018 · 8 comments
Closed

Investigate rAF scheduling - seems unnecessary #216

Andarist opened this issue Jul 21, 2018 · 8 comments

Comments

@Andarist
Copy link
Owner

It's only done in controlled mode anyway - uncontrolled doesnt use it and we dont have any issues reported with it.

@stevenla
Copy link

stevenla commented Oct 2, 2018

Without rAF, the component forces a relayout on componentDidMount. Something like fastdom would help mitigate this.

screen shot 2018-10-02 at 12 55 35 pm

@Andarist
Copy link
Owner Author

Andarist commented Oct 2, 2018

Thanks for pointing out to me that I can actually use profiler to check out the difference between both implementations! Need to investigate it in free time.

@Andarist
Copy link
Owner Author

Andarist commented Oct 4, 2018

Could u describe how did you test that? I cannot observe this with JavaScript Profiler in Chrome.

I've tested both versions - with and without rAF:

Keep in mind that rAF was used only in componentDidUpdate, componentDidMount was unaffected by it - calculateNodeHeight + setState were called there unconditionally.

@Andarist
Copy link
Owner Author

Andarist commented Oct 7, 2018

@stevenla I'd like to do a release without rAF scheduling soon, could u comment on this? If this could in fact cause a perf regression I wouldnt like to do it obviously, but at the moment I'm not able to reproduce it, so your input is extremely valuable for me

@stevenla
Copy link

let me try to get a smaller repro case—that was from the app.

@Andarist
Copy link
Owner Author

Cool! I'll be waiting for it then before releasing the change as latest.

@Andarist
Copy link
Owner Author

@stevenla any news?

@Andarist
Copy link
Owner Author

Andarist commented Dec 3, 2018

I've released a version without this scheduling, I couldnt find any reason to keep it - couldnt reproduce any perf issues without this scheduling. Therefore Im closing this issue now, if you bring up some data about it I will be glad to look at it and Im not strictly against having this scheduling in the library (it there is a reason behind it).

@Andarist Andarist closed this as completed Dec 3, 2018
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