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.

Sign up for GitHub

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
Merged

fix(4882): reduce ResizerObserver looping even further #4982

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/visualization/types/SimpleTable/PagedTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,13 +239,14 @@ const PagedTable: FC<Props> = ({result, properties}) => {
}

let timeout
let animationFrameID
const resizer = new ResizeObserver(entries => {
if (timeout) {
clearTimeout(timeout)
}

timeout = setTimeout(() => {
requestAnimationFrame(() => {
animationFrameID = requestAnimationFrame(() => {
setHeight(entries[0].contentRect.height)
})
}, 200)
Comment on lines 248 to 252
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.

Expand All @@ -263,6 +264,7 @@ const PagedTable: FC<Props> = ({result, properties}) => {

return () => {
resizer.disconnect()
cancelAnimationFrame(animationFrameID)
if (timeout) {
clearTimeout(timeout)
}
Expand Down