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

fix: "ResizeObserver loop limit exceeded" #23

Merged

Conversation

FezVrasta
Copy link
Contributor

Copies the approach used in airbnb/visx#335 to fix a "ResizeObserver loop limit exceeded" error

@FezVrasta FezVrasta force-pushed the fix/resizeobserver-loop-limit-exceeded branch from a6f512a to 3bd051d Compare July 30, 2019 17:01
@petyosi petyosi self-assigned this Jul 31, 2019
@petyosi
Copy link
Owner

petyosi commented Jul 31, 2019

Hey @FezVrasta - do you have a repro for the issue somewhere? Thanks!

@petyosi petyosi self-requested a review July 31, 2019 07:05
@petyosi petyosi added the bug Something isn't working label Jul 31, 2019
@FezVrasta
Copy link
Contributor Author

No I'm sorry, it happened in a fairly complex non-OSS app, but you should be able to track down the issue following the PR I linked in the commit

@petyosi
Copy link
Owner

petyosi commented Jul 31, 2019

Hey @FezVrasta ,

I tested the change locally and it broke the grouped example. I traced the problem to some height measurements being dropped due to the component being unmounted and canceling the animation frame request.

What worked for me was to remove the

useEffect(() => () => window.cancelAnimationFrame(animationFrameID.current))

call. I think this should bring no harm, but I might be wrong (for example, measuring items which are no longer in the DOM). We will see.

Can you please test this in your setup? Thank you.

@FezVrasta
Copy link
Contributor Author

FezVrasta commented Aug 1, 2019

I can't seem to be able to reproduce the problem if I comment out that line, but I'm not sure what could happen if onResize gets called while the component is unmounted. If that callback performs a check to make sure it's not running on a detached/destroyed node then everything should be okay.

Sorry I did forget to add the [] as second argument of useEffect, may you check again and let me know if it works properly now?

@FezVrasta FezVrasta force-pushed the fix/resizeobserver-loop-limit-exceeded branch from 3bd051d to c0f1d6d Compare August 1, 2019 11:00
@petyosi
Copy link
Owner

petyosi commented Aug 1, 2019

Yeah, It is reproducible only in grouped mode with footer set, since multiple measurements are performed. I will examine it further and figure out something.

@FezVrasta
Copy link
Contributor Author

How do you run the docs locally with a local version of react-virtuoso? Do I have to switch to gh-pages and manually copy the build?

@petyosi
Copy link
Owner

petyosi commented Aug 1, 2019

This is the site: https://github.com/petyosi/react-virtuoso-site

I keep it as a peer directory of the react-virtuoso repo, (the dir is called lib):
https://github.com/petyosi/react-virtuoso-site/blob/master/package.json#L32
To test the lib changes with the demos, I run yarn start in the lib directory, and then yarn start in the site directory.

Thank you for your effort!

@FezVrasta FezVrasta closed this Aug 1, 2019
@FezVrasta FezVrasta reopened this Aug 1, 2019
@FezVrasta
Copy link
Contributor Author

FezVrasta commented Aug 1, 2019

Thanks I got it working, but I can't reproduce any issue with the grouped example, what's the issue I should see?

Here's a gif of what I see:

Kapture 2019-08-01 at 13 29 24

@petyosi
Copy link
Owner

petyosi commented Aug 1, 2019

On my side, when I load the example (without any interaction - resize, scroll, etc), the items do not get loaded. I only see the header and the button. The reason is that one measurement gets dropped.

@FezVrasta
Copy link
Contributor Author

May I know more about your environment? I wonder if it's OS/browser/version specific.

@petyosi
Copy link
Owner

petyosi commented Aug 1, 2019

Chrome 75 on MacOS.

@FezVrasta
Copy link
Contributor Author

Ok, same here, I'm not sure what's going on then...

@petyosi
Copy link
Owner

petyosi commented Aug 1, 2019

Did you rebase on v0.10? I made one minor change in the hooks behavior there, and that is the only problem I can think of.

Anyway, will check again on my side later this week.

Copies the approach used in airbnb/visx#335 to fix a "ResizeObserver loop limit exceeded" error
@FezVrasta FezVrasta force-pushed the fix/resizeobserver-loop-limit-exceeded branch from c0f1d6d to 118da4c Compare August 1, 2019 13:16
@FezVrasta
Copy link
Contributor Author

I just rebased but I can't still reproduce your issue

@petyosi petyosi merged commit 118da4c into petyosi:master Aug 1, 2019
@petyosi
Copy link
Owner

petyosi commented Aug 1, 2019

Whatever it was, it works now. Sorry for the hassle. published v0.10.1 with the fix.

Thank you once again,

@petyosi petyosi removed their request for review August 1, 2019 17:03
@FezVrasta FezVrasta deleted the fix/resizeobserver-loop-limit-exceeded branch August 1, 2019 17:15
@FezVrasta
Copy link
Contributor Author

I'm experiencing the same issue again, I guess I didn't fix it completely 😢

@petyosi
Copy link
Owner

petyosi commented Sep 18, 2019

I haven't changed anything in the area since this PR. Something which might help me is the stack trace (and, of course, a repro would be awesome). Contact me on my email if I can observe the problem somewhere.

Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants