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

ResizeObserver loop limit exceeded #2588

Closed
giniedp opened this issue Aug 22, 2018 · 27 comments
Closed

ResizeObserver loop limit exceeded #2588

giniedp opened this issue Aug 22, 2018 · 27 comments

Comments

@giniedp
Copy link

giniedp commented Aug 22, 2018

I'm submitting a ...

[x] bug report => see 'Providing a Reproducible Scenario'
[] feature request => do not use Github for feature requests, see 'Customers of ag-Grid'
[] support request => see 'Requesting Community Support'

Current behavior
There is no issue during runtime but running our test suite (ci, docker) tests randomly fail because of the following error

ResizeObserver loop limit exceeded

Please tell us about your environment:

  • ag-Grid version: 18.1.1

  • Browser:
    Chrome Headless 68 stable (66, 67 and 69 beta also fail)

  • Language: Typescript 2.8.3 / ES5

Looking through the ag-grid source code i could not spot any issue. This
https://github.com/ag-grid/ag-grid/blob/18.1.1/packages/ag-grid/src/ts/resizeObserver.ts#L396
looks actually ok.

Still i was wondering if the callback itself might be causing a resize event and then run into a loop, but the gotcha here https://developers.google.com/web/updates/2016/10/resizeobserver#gotcha
says that this case is actually protected

However, this does not seem to be the case as this plunkr demonstrates
https://jsfiddle.net/que_etc/ba1ad26e/
so i changed the ag-grid code at
https://github.com/ag-grid/ag-grid/blob/master/packages/ag-grid/src/ts/resizeObserver.ts#L396
from this

for (const entry of entries) {
    callback(entry);
}

to this

for (const entry of entries) {
    setTimeout(function() {
        callback(entry);
    })
}

with the result that the error does not occur and the tests don't fail any more.

@outofgamut
Copy link

any plans on a fix for this coming out in a patch release?

@giniedp
Copy link
Author

giniedp commented Aug 31, 2018

i was told, its being tracked as a bug now.
see https://www.ag-grid.com/ag-grid-pipeline/
Key: AG-2028

@giniedp
Copy link
Author

giniedp commented Sep 13, 2018

19.0.0 is out and AG-2028 had been marked as fixed although i can not find it any more. However our ci pipeline still fails randomly

@giniedp
Copy link
Author

giniedp commented Sep 13, 2018

Looking at the source code, there has been some work on the resize observer, but the issue has not been addressed. The old polyfill observer is gone, but the issue was with the native ResizeObserver when using Chrome and that is processing the callback directly as before.

https://github.com/ag-grid/ag-grid/blob/b19.0.0/packages/ag-grid-community/src/ts/misc/resizeObserverService.ts#L17

Changing the source code from

const resizeObserver = new (<any>window).ResizeObserver(callback);

to

const resizeObserver = new (<any>window).ResizeObserver(() => 
frameworkFactory.setTimeout(callback));

fixes the issue and all tests pass

@Laurensvdw
Copy link

Laurensvdw commented Sep 14, 2018

I can confirm the error still exists.
For me the sizeColumnsToFit() method is causing the error, resutling in the application to break. It is because I have a left side navigation and closing/opening/pinning the navigation makes the resizer run wild, as the main content holding the grid resizes (it is random, it does not occur every time....).

@giniedp Thanks for the solution, hopefully this time they will copy past your code ;)

@giniedp
Copy link
Author

giniedp commented Sep 14, 2018

so i have been digging around and found the following.

The resize observer is internally used in two places

The first one is here
https://github.com/ag-grid/ag-grid/blob/19.0.1/packages/ag-grid-community/src/ts/gridCore.ts#L126
This simply delegates to the event dispatcher to dispatch the resize event for ag-grid consumers. There seems to be no further internal logic attached to that event. However, if an ag-grid consumer would subscribe to the resize event and cause some sort of layout change (adding or removing a css class ?!) this most likely would end up in an exception.

The other one is here
https://github.com/ag-grid/ag-grid/blob/19.0.1/packages/ag-grid-community/src/ts/gridPanel/gridPanel.ts#L337
which in the end performs some layout operations. I was able to spot the problematic calls which are

In my observation as soon as some padding is added to eFullWidthViewportWrapper or eRightContainer or eLeftContainer the error occurs. The weird thing is that a padding to those elements does not affect the size of the eBodyViewport which is being watched.

I still suggest to change the resize observer subscription as i initially proposed so that the layout operations are delayed into the next cycle. Doing so fixes the current issue and has no observable side effects and the resize observer does not trigger a second time.

@giniedp
Copy link
Author

giniedp commented Sep 14, 2018

@Laurensvdw is your project anyhow publicly available where the issue can be reproduced? how often does it occur? ag-grid team asks for a reproducable example. For me it is only happening on a ci server and that is a bit hard to isolate, extract and reproduce.

@Laurensvdw
Copy link

@giniedp Nice "Pink Panter" spy work ;)
I will try to reproduce with a plunker. As it is not possible to share the website (government restricted)...

@giniedp
Copy link
Author

giniedp commented Sep 14, 2018

@Laurensvdw i've got a vanilla js ag-grid setup on stackblitz
https://stackblitz.com/edit/js-ag-grid
but i can't break the resize

@Laurensvdw
Copy link

@giniedp It seems that it is not reproducible, my layout is much more complex than this....
Link to stackblitz example

Landing on the component and clicking on the button "Toggle menu state", most of the times cause the error, but not in the example :(

@giniedp
Copy link
Author

giniedp commented Sep 14, 2018

thx anyway for trying

@giniedp
Copy link
Author

giniedp commented Sep 18, 2018

i was able to reproduce the behavior. Repository is located here
https://github.com/giniedp/ag-grid-ro-issue

@Laurensvdw would you mind to review my reproduction and tell if it also breaks on your side?

meanwhile i am reporting back to ag-grid team.

@Laurensvdw
Copy link

Same outcome.

image

After limiting columns in the spec file the errors are gone! =:)

@giniedp
Copy link
Author

giniedp commented Sep 18, 2018

I was able to sort out my scenario. We did not include stylesheets into the tests. This might introduce a layout mess into ag-grid. Adding stylesheets does let all tests pass again.

This Leaves @Laurensvdw issue still unresolved.

@Laurensvdw
Copy link

I think it is due to the nature of our "complex" layout. The side navigation has different modes which make the actual content resize. Even if we only have 4 columns, it still happens.

My best guess is that it has something to do with the resizing (animation) of the content, which triggers the grid resizer (for every pixel?).

I hope the team of ag-grid just implements your solution, and that it hopefully resolves the issue.

@giniedp
Copy link
Author

giniedp commented Sep 19, 2018

yes, if there are animations, the new behavior of ag-grid will trigger the resize event once per animation step. I have changed my example on stackblitz https://stackblitz.com/edit/js-ag-grid but still cant break it.

@Laurensvdw
Copy link

Used your column/row generation loops and added a transition for the margin-left when toggling the side navigation (to make the resizing even heavier). I can't break it either... I am starting to fear we are dealing with magic... ☠️

The example once again

@makinggoodsoftware
Copy link
Contributor

Hi

Just to let you know that ag-grid staff is now going to monitor this thread to help find an answer to this issue (hence the ag-grid engaged label)

Thanks for all the feedback

@makinggoodsoftware
Copy link
Contributor

Hi,

We appreciate that there is an issue seen by many users, but we are struggling to reproduce in our end.

Thanks at @giniedp for the repository.

We are going to add a mechanism to opt-out of the native resize observer

This is going to be in our next release

AG-2066 Create an opt out of the native ResizeObserver

https://www.ag-grid.com/ag-grid-pipeline/

Note that we can't still fully reproduce this in our end, so we are not sure this will fully help you guys.

If you do come up with a simple reproducible scenario, please let us know

@Laurensvdw
Copy link

Hey Alberto,

Thanks for following it up and providing a possible solution. We will implement the solution when it is available.

The "bug" is indeed very hard to reproduce, in our case we have it 20% of the time, so no logic there...

@stepet80
Copy link

I can confirm the error still exists.
For me the sizeColumnsToFit() method is causing the error, resutling in the application to break. It is because I have a left side navigation and closing/opening/pinning the navigation makes the resizer run wild, as the main content holding the grid resizes (it is random, it does not occur every time....).

@giniedp Thanks for the solution, hopefully this time they will copy past your code ;)

It also works if you do sizeColumnsToFit() before you assign the datasource...

@Laurensvdw
Copy link

Reminds me of posting my progress...
For me the issue remained when I removed the sizeColumnsToFit() method (maybe @stepet80 his solution fixes this).

The real solution, for my environment, was to declare a certain height on the wrapping container. Apparently this was missing... Like if you would place the ag-grid directly on the body and you wouldn't declare "height:100%" on the body. Then the grid would do weird things like not showing any rows as the height cannet be retrieved or something like that...

@sontek
Copy link

sontek commented Nov 15, 2018

I'm getting this with:

import React from "react";
import ReactDataGrid from 'react-data-grid';
import styles from "../../sass/materials/datagrid.scss";
import { AgGridReact } from 'ag-grid-react';


class DataGrid extends React.PureComponent {
    constructor(props) {
        super(props);

        this.state = {
            columnDefs: [
                {headerName: "Make", field: "make"},
                {headerName: "Model", field: "model"},
                {headerName: "Price", field: "price"}

            ],
            rowData: [
                {make: "Toyota", model: "Celica", price: 35000},
                {make: "Ford", model: "Mondeo", price: 32000},
                {make: "Porsche", model: "Boxter", price: 72000}
            ]
        }
    }

    render() {
        return (
            <div className={styles.dataGrid}>
                <AgGridReact
                    columnDefs={this.state.columnDefs}
                    rowData={this.state.rowData}>
                </AgGridReact>
            </div>
        )
    }
}

export default DataGrid;

@sontek
Copy link

sontek commented Nov 15, 2018

We had this problem when we were using ResizeObserver in newer versions of Chrome as well and we just throttled it.

        this.resizeObserver = new ResizeObserver(
            debounce(this.handleResize, 10)
        );

and it worked perfectly.

@frankwallis
Copy link

frankwallis commented Dec 5, 2018

For us this appears to be triggered when the side scrollbar is initially visible and then some rows are removed from the grid, so that the scrollbar is not visible any more.

We have pinned columns on the right, so I was able to workaround the issue by making the right-hand scrollbar always visible:

.ag-theme-balham .ag-pinned-right-cols-viewport {
  overflow-y: scroll;
}

@makinggoodsoftware
Copy link
Contributor

Hi guys,

Quick update, the opt out to not use the resizer is going to be introduced in v20.1 ETA 4/5 weeks, sorry for the long delay

Thanks

@makinggoodsoftware
Copy link
Contributor

Hi,

Thank you for your email. We are aware of this issue and have a feature request in our backlog:

You can follow the status of feature requests, bugs and releases using our pipeline:
https://www.ag-grid.com/ag-grid-pipeline/

If you are an ag-Grid Enterprise Customer, please raise this in our Zendesk support system - contact [email protected] for access. This is our primary channel for Support.

This is the reference and summary of the relevant issue:

AG-2066 Create an opt out of the native ResizeObserver

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

No branches or pull requests

7 participants