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

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

Open
sniederb opened this issue Jun 22, 2016 · 7 comments · May be fixed by #526
Open

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

sniederb opened this issue Jun 22, 2016 · 7 comments · May be fixed by #526

Comments

@sniederb
Copy link

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
Copy link
Contributor

fonji commented Jun 22, 2016

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

@sniederb
Copy link
Author

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
Copy link
Contributor

fonji commented Jun 22, 2016

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
Copy link
Author

sniederb commented Jun 22, 2016

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
Copy link
Collaborator

glenpike commented Sep 5, 2016

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
Copy link
Author

sniederb commented Sep 6, 2016

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
Copy link
Collaborator

glenpike commented Sep 6, 2016

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.

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

Successfully merging a pull request may close this issue.

3 participants