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

dgrid cell handlers almost never cleaned #1211

Closed
jmuransky opened this issue Nov 26, 2015 · 0 comments
Closed

dgrid cell handlers almost never cleaned #1211

jmuransky opened this issue Nov 26, 2015 · 0 comments

Comments

@jmuransky
Copy link

TL;DR the problem is with storing cell event handlers in array, that gets virtually never cleaned up - even when actual cells are created/released fairly regularly.

So the issue is with Editor.js with code in _configureEditorColumn method's column.renderCell anonymous function containing code:

if (!options || !options.alreadyHooked) {
    self._editorColumnListeners.push(
        on(cell, editOn, function () {
            self._activeOptions = options;
            self.edit(this);
        })
    );
}

Here we can see, that cell's event handlers are stored in grid's "_editorColumnListeners". This array of handlers belongs to columns - and therefore is cleared only when "_editorStructureCleanup" method is called (as found out via project-wide search) - so basicly only when grid structure changes.
Now when we consider, that someone could view same grid for prolonged time (like with using pagination and browsing pages of grid => cells get created and discarded), then we can see, that the number of handlers attached on unused cells increases and memory consumption with it...

Correct solution would be to bind cell event listeners to some new array called like "_editorCellListeners", that would be created for each row in e.g. insertRow and cleanup upon call to e.g. "removeRow".

The only problem with this solution is finding correct row by cell - there is no direct referrence and looking up row by traversing DOM feels dirty/slow.

I am willing to create full solution to this problem, but I don't know all the details of API and the overall picture of the solution, so I might be missing some elegant feature/place that would make this fix cleaner (my very direct and possibly wrong approach would be to edit Grid.js "createRowCells" function for every cell to have direct link to it's row AND storing "_editorCellListeners" array of listeners directly on row HTML widget).

maier49 added a commit to maier49/dgrid that referenced this issue Dec 23, 2015
maier49 added a commit to maier49/dgrid that referenced this issue Dec 29, 2015
maier49 added a commit to maier49/dgrid that referenced this issue Dec 29, 2015
@maier49 maier49 closed this as completed in 8c3d0c6 Jan 6, 2016
maier49 added a commit that referenced this issue Jan 6, 2016
(cherry picked from commit 8c3d0c6)

Modified for backport to remove refreshCell considerations (not in 0.4).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants