gvas / knockout-jqueryui

Knockout bindings for the jQuery UI widgets.
http://gvas.github.com/knockout-jqueryui/
MIT License
103 stars 38 forks source link

Tabs binding causes memory-leak #3

Closed gabimousa closed 9 years ago

gabimousa commented 11 years ago

The Tabs binding causes serious memory leak.

I have a observableArray the contains tabObjects. Now i can add and remove tabs from this array.

If you watch the application in chrome task manager the memory keeps growing even after removing the tabs from the observableArray.

this is a serious issue for me because i am building a single page app that will be running for a long time without refreshing. It retrieves it's data by making ajax calls.

look at this fiddle http://jsfiddle.net/Pyf6c/4/

gvas commented 11 years ago

I've recreated your fiddle with pure jQuery/jQuery UI, and it also leaks memory. This seems to me like a bug in jQuery/jQuery UI. If someone can assure that or sees a flaw in the jsfiddle please write a comment. Thanks!

http://jsfiddle.net/smqFN/

gvas commented 11 years ago

assure = confirm

gabimousa commented 11 years ago

That's right. the problem lies within jquery refresh function.

Is it possible to replace the refresh action with a destroy and creation of the widget without screwing up your extension.

see the code below:

subscribeToRefreshOn = function (widgetName, element, bindingValue, widgetOptionsAndEvents) {
    /// <summary>Creates a subscription to the refreshOn observable.</summary>
    /// <param name='widgetName' type='String'>The widget's name.</param>
    /// <param name='element' type='DOMNode'></param>
    /// <param name='bindingValue' type='Object'></param>

    if (ko.isObservable(bindingValue.refreshOn)) {
        ko.computed({
            read: function () {
                bindingValue.refreshOn();
                //$(element)[widgetName]('refresh');
                $(element)[widgetName]('destroy');
                $(element)[widgetName](unwrapProperties(widgetOptionsAndEvents));
            },
            disposeWhenNodeIsRemoved: element
        });
    }
};

and then calling it like so

                if (options.hasRefresh) {
                    subscribeToRefreshOn(widgetName, element, value, widgetOptionsAndEvents);
                }
gvas commented 9 years ago

The refresh method's memory leak is fixed in jQuery UI 1.11.2 jQuery UI 1.11.1 still leaks memory: http://jsfiddle.net/gvas/51nxkrjm/ jQuery UI 1.11.2 doesn't: http://jsfiddle.net/gvas/pLyud199/