-
Notifications
You must be signed in to change notification settings - Fork 166
Fix error ResizeObserver loop limit exceeded #1647
Fix error ResizeObserver loop limit exceeded #1647
Conversation
Is there a terra issue logged for this? |
Default | ||
</div> | ||
`; | ||
exports[`should render a default component 1`] = `<div />`; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto Generated.
There was a problem hiding this comment.
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();
});
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
}
}
There was a problem hiding this comment.
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.
@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 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 |
@MadhavaNaresh if you are seeing this issue again, it would be best to log a new issue. |
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.