powmedia / backbone-forms

Form framework for BackboneJS with nested forms, editable lists and validation
MIT License
2.17k stars 415 forks source link

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

Open sniederb opened 8 years ago

sniederb commented 8 years ago

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.

fonji commented 8 years ago

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

sniederb commented 8 years ago

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 ?

fonji commented 8 years ago

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

sniederb commented 8 years ago

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;
        }
    }

};
glenpike commented 7 years ago

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?)

sniederb commented 7 years ago

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.

glenpike commented 7 years ago

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.