backbone-forms icon indicating copy to clipboard operation
backbone-forms copied to clipboard

Memory leak in Form.editors.List.setValue()

Open sniederb opened this issue 8 years ago • 7 comments

On the master, Form.editors.List.prototype.setValue() is

setValue: function(value) {
  this.value = value;`
  this.render();
},

The render() causes the view's $el to be replaced, without detaching the previous elements. I'm currently overwriting this:

Backbone.Form.editors.List.prototype.setValue = function(value) {
    this.value = value;
    if (!$.isArray(value)) {
        throw new Error("List value must be an array");
    }

    for (var i = 0; i < value.length; i++) {
        this.items[i].setValue(value[i]);
    }
};

If you agree with the change, would be nice to see that patched.

sniederb avatar Jun 22 '16 12:06 sniederb

What if value.length != this.items.length?

fonji avatar Jun 22 '16 13:06 fonji

I'm not saying my Overwrite is bug-free, but the current setvalue() implementation definitely causes problems. Probably for for-loop would need to work with addItem / removeItem ?

sniederb avatar Jun 22 '16 13:06 sniederb

Probably, yes. I was just concerned because you said that you overwrote it, so I thought you might have a bug in production somewhere.

fonji avatar Jun 22 '16 14:06 fonji

Sorry about the misleading wording. I want to use the backbone forms JS unchanged, so I'm overwriting some prototype functions in my own code. My suggestion (haven't tested it):

Backbone.Form.editors.List.prototype.setValue = function(value) {
    this.value = value;
    if (!$.isArray(value)) {
        throw new Error("List value must be an array");
    }

    for (var i = 0; i < value.length; i++) {
        if (this.items.length < i) {
            this.items[i].setValue(value[i]);   
        }
        else {
            this.addItem(value[i], false);
        }
    }

    while (this.items.length > value.length) {
        this.removeItem(this.items[this.items.length - 1]); 
        if (value.length === 0 && this.items.length === 1) {
            // beware that removeItem() will re-add a new item 
            // if 'this.items' is empty
            break;
        }
    }

};

sniederb avatar Jun 22 '16 15:06 sniederb

I've been trying to reproduce this at a higher level - possibly misunderstand what you're trying to achieve. Anyway, I've created a JSFiddle which calls setValue on the field - but nothing seems to change in the editors. Is that what you mean by "not detached", or are you talking about something else (perhaps there's 2 bugs here, or I'm testing something that shouldn't normally happen?)

glenpike avatar Sep 05 '16 22:09 glenpike

I've taken your JSFiddle and extended it a bit to this one.

I'd expect the list to re-render on the form.setValue() call, for seem reason that's not happening. If the list get's re-rendered, the .change() handler doesn't get called anymore, because this.$el was replaced.

sniederb avatar Sep 06 '16 06:09 sniederb

I see what you mean now.

The other problem is that if you console.log form.getValue() after clicking the button, you have 6 items in your array - that's #372 all over. I think I'll try to look at that pull-request to see if we can solve your issue too.

glenpike avatar Sep 06 '16 18:09 glenpike