Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

Fix error ResizeObserver loop limit exceeded #1647

Closed
wants to merge 5 commits into from
Closed

Fix error ResizeObserver loop limit exceeded #1647

wants to merge 5 commits into from

Conversation

ap056120
Copy link

Summary

Resolves the ResizeObserver loop limit exceeded error that is thrown when Responsive elements are loaded.

Additional Details

When a ResponsiveElement is mounted for the first time, the ResizeObserver receives an event with the size of the ResponsiveElement’s parent, which the ResponsiveElement uses to render the correct component in place of the div it used to initially get the ref. In cases where the ResponsiveElement’s parent is wrapped to its contents, the act of rendering the actual responsive component will cause another resize event to be triggered, which the ResizeObserver will detect as a potential infinite loop. The error thrown by the ResizeObserver is intended as a warning, doesn’t actually prevent infinite loops, and the ResponsiveElement already has logic to prevent accidental infinite loops, so the error just gets in the way. In our case, however, other components we are consuming don’t realize this and overreact to the error, so we need to suppress it. We can fix this by delaying the second rendering (the first one where an actual child component is rendered) for one frame after the first rendering with the ref div.

The 'ResizeObserver loop limit exceeded' error is not thrown in the JavaScript console of a browser, but it can be caught using a simple extension that is available for Chrome/Firefox:

The error can currently be demonstrated in Kaiju as well. This JS Fiddle demonstrates that the loop limit is 0, and any size modification within the resize handler triggers the warning.

Documentation for ResizeObserver

This GitHub issue proposes an alternate solution to the loop limit problem, but the current implementation of the ResizeObserver doesn’t allow that to solution to work.

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-1647 June 26, 2018 12:53 Inactive
@emilyrohrbough
Copy link
Contributor

Is there a terra issue logged for this?

Default
</div>
`;
exports[`should render a default component 1`] = `<div />`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think any tests should have been affected by this change, was this file auto generated or manually updated?

Copy link
Author

Choose a reason for hiding this comment

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

Auto Generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

This snapshot dropped the default text. I would try compiling and running the tests again.

If this change is a result of the changes in ResponsiveElement.jsx it is non-passive and is not rendering the element.

It looks like this test renders the initial div but never renders the defaultElement.

it('should render a default component', () => {
  const responsiveElement = shallow(<ResponsiveElement defaultElement={<div>Default</div>} />);
  expect(responsiveElement).toMatchSnapshot();
});

Choose a reason for hiding this comment

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

It doesn't render the default text in the same frame, likely due to the call to requestAnimationFrame. Are we expecting the default text to be rendered at the same time as the initial div?

Copy link
Contributor

Choose a reason for hiding this comment

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

The call to freeze the repaint breaks this test. My concern is that it will also cascade and break a significant amount of consumer tests down the chain.

Copy link
Contributor

@StephenEsser StephenEsser Jun 26, 2018

Choose a reason for hiding this comment

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

It also breaks all snapshot tests for components that use the responsive element, as the expected element will never be shallow rendered correctly.

Choose a reason for hiding this comment

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

An alternative to the requestAnimationFrame would be to call handleResize before the resizeObserver is attached with the current size of the of the container. Would this be acceptable?

  componentDidMount() {
    if (this.container) {
      this.handleResize(this.container.innerWidth);
      this.resizeObserver = new ResizeObserver((entries) => { this.handleResize(entries[0].contentRect.width); });
      this.resizeObserver.observe(this.container);
    } else {
      this.handleResize(window.innerWidth);
      window.addEventListener('resize', this.handleWindowResize);
    }
  }

Copy link
Contributor

@StephenEsser StephenEsser Jun 26, 2018

Choose a reason for hiding this comment

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

Calling handResize before observe might cause a triple render.

Edit: Verified this does cause a triple render.

@mjhenkes mjhenkes temporarily deployed to terra-core-deployed-pr-1647 June 26, 2018 14:16 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-1647 June 26, 2018 16:42 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-1647 June 26, 2018 16:44 Inactive
@shinepd-zz
Copy link

@emilyrohrbough we didn't log an issue for this change. I'm not aware of an open issue in terra-core pertaining to this change.

@mjhenkes
Copy link
Contributor

@ap056120 @shinepd, We've talked internally and we've come up with another fix. We're going to close this pull request and open up one of our own by end of day tomorrow at the latest. Thanks for the debugging time you guys put into this.

@MadhavaNaresh
Copy link

@mjhenkes Wanted to know if this issue was fixed. I'm getting the ResizeObserver loop error when integrating a react component to a mpages view through workflow_server

@mjhenkes
Copy link
Contributor

@MadhavaNaresh if you are seeing this issue again, it would be best to log a new issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants