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(4882): reduce ResizerObserver looping even further #4982

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

wiedld
Copy link
Contributor

@wiedld wiedld commented Jul 7, 2022

Part of #4882

Part of bug squashing mission.

Bug:

  • general window.onerror triggered via the ResizeObserver
  • What this is:
  • Previously done:
    • we are already doing 2 (out of 3) code patterns to reduce the triggering occurrences. We can also do the 3rd one too (cancel animation frame).

In addition to this PR, going to be requesting permission to start filtering out (ignoring) this error in honeybadger.

@wiedld wiedld requested a review from a team as a code owner July 7, 2022 04:58
Comment on lines 248 to 252
timeout = setTimeout(() => {
requestAnimationFrame(() => {
animationFrameID = requestAnimationFrame(() => {
setHeight(entries[0].contentRect.height)
})
}, 200)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know why this RAF is in a setTimeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a general pattern when using ResizeObserver --> to then trigger a callback (e.g. set height of inner component) which then may recursively trigger ResizeObserver again. This is a known problem, which is why browsers do not surface these errors in the console...but it does still gets report in window.onerror() --> which we pipe to honeybadger.

Here is an issue ticket from WICG, which includes this^^ conclusion, as well as many code samples where a community of devs have listed all the code patterns which they use to decrease the number of recursive triggers (although is impossible to eliminate). One of the patterns is to have the callback action be encapsulated in a setTimeout() (https://github.com/WICG/resize-observer/issues/38#issuecomment-422085926), which queues it up at the back of the javascript runtime macrotask queue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the patterns is to have the callback action be encapsulated in a setTimeout() which queues it up at the back of the javascript runtime macrotask queue.

Thanks for the explanation. Is the 200 millisecond delay significant? Could this could this be achieved with a delay of 1 or 0 milliseconds? The example doesn't set the delay value explicitly, which defaults it to a value of 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout was put in previously. I can only guess the 200ms is arbitrary. The 0 is sufficient for insertion at the back of the macrotask queue.

@wiedld wiedld merged commit 81f3f67 into master Jul 7, 2022
@wiedld wiedld deleted the 4882/cancel-animation-frame branch July 7, 2022 17:35
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

Successfully merging this pull request may close these issues.

2 participants