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

Expressions never removed from observable._expressions #90

Closed
henrikekblad opened this issue Sep 25, 2015 · 5 comments
Closed

Expressions never removed from observable._expressions #90

henrikekblad opened this issue Sep 25, 2015 · 5 comments
Labels

Comments

@henrikekblad
Copy link

Hi,

We notice that expressions are never removed from observable._expressions. Is this correct?

https://jsfiddle.net/bynxrd0j/

<div data-query="each(selectedItems)">
    {{$this}}
</div>
<pre data-query="each(output)">
    {{$this}}
</pre>
var Model = function() {
    var self = this;
    this.item1 = blocks.observable("ITEM1");
    this.item2 = blocks.observable("ITEM2");
    this.items = [this.item1, this.item2];
    this.selectedItems = blocks.observable([this.item1]);
    this.output = blocks.observable([]);
    var i = 0;
    setInterval(function() {
        var item = self.items[i++ % 2];
        self.output.add(i+" selecting item: "+item); 
        self.selectedItems.removeAll();
        self.selectedItems.add(item);
        self.output.add(i+" item1._expressions.length is now: "+self.item1._expressions.length);
    }, 1000);
};

blocks.query(new Model);
ITEM1
    1 selecting item: ITEM1
    1 item1._expressions.length is now: 2
    2 selecting item: ITEM2
    2 item1._expressions.length is now: 2
    3 selecting item: ITEM1
    3 item1._expressions.length is now: 3
    4 selecting item: ITEM2
    4 item1._expressions.length is now: 3
    5 selecting item: ITEM1
    5 item1._expressions.length is now: 4
    6 selecting item: ITEM2
    6 item1._expressions.length is now: 4
    7 selecting item: ITEM1
    7 item1._expressions.length is now: 5
    8 selecting item: ITEM2
    8 item1._expressions.length is now: 5
@astoilkov
Copy link
Owner

Hi @henrikekblad,

You are absolutely correct. I am marking this as bug. The expressions should be removed from the ._expressions array.

@astoilkov astoilkov added the bug label Sep 25, 2015
@henrikekblad
Copy link
Author

Thanks, note that this probably also means DOM elements never gets garbage collected correctly.

@astoilkov
Copy link
Owner

Indeed there was such a bug but it was resolved. You could see the commit 86b559d

@tomas2t
Copy link

tomas2t commented Sep 25, 2015

I'm working on the same project as @henrikekblad and you are right. The "DOM elements leak" was nothing more than a lazy guess by me.

I have updated the jsfiddle to show that you are correct, and as a result an exception will be throw instead when trying to evaluate the "dom-less" expression... :)
https://jsfiddle.net/bynxrd0j/1/

<button data-query="click(update)">Click me after a while to get an exception because  ElementsData.data(expression.elementId) returns undefined...</button>
<div data-query="each(selectedItems)">
    {{$this}}
</div>
<pre data-query="each(output)">
    {{$this}}
</pre>
var Model = function() {
    var self = this;
    this.item1 = blocks.observable("ITEM1");
    this.item2 = blocks.observable("ITEM2");
    this.items = [this.item1, this.item2];
    this.selectedItems = blocks.observable([this.item1]);
    this.output = blocks.observable([]);
    this.update = function() {
        try {
            self.output.add("Button clicked! Will now update the value of item1!");
            self.item1("ITEM1 "+Date.now());
        } catch (err) {
            self.output.add("Yea, got it:");
            self.output.add(err.stack);
            throw err;
        }
    };
    var i = 0;
    setInterval(function() {
        var item = self.items[i++ % 2];
        self.output.add(i+" selecting item: "+item); 
        self.selectedItems.removeAll();
        self.selectedItems.add(item);
        self.output.add(i+" item1._expressions.length is now: "+self.item1._expressions.length);
    }, 1000);
};

blocks.query(new Model);
    1 selecting item: ITEM1
    1 item1._expressions.length is now: 2
    2 selecting item: ITEM2
    2 item1._expressions.length is now: 2
    3 selecting item: ITEM1
    3 item1._expressions.length is now: 3
    Button clicked! Will now update the value of item1!
    Yea, got it:
    TypeError: Cannot read property 'dom' of undefined
    at updateExpression (https://cdn.rawgit.com/astoilkov/jsblocks/master/dist/blocks-source.js:7405:57)
    at Function.blocks.eachRight (https://cdn.rawgit.com/astoilkov/jsblocks/master/dist/blocks-source.js:183:9)
    at Function.blocks.extend.fn.base.update (https://cdn.rawgit.com/astoilkov/jsblocks/master/dist/blocks-source.js:7399:18)
    at blocks.observable.observable [as item1] (https://cdn.rawgit.com/astoilkov/jsblocks/master/dist/blocks-source.js:7281:20)
    at update (https://fiddle.jshell.net/bynxrd0j/1/show/:34:15)
    at https://cdn.rawgit.com/astoilkov/jsblocks/master/dist/blocks-source.js:7031:22
    at Function.blocks.each (https://cdn.rawgit.com/astoilkov/jsblocks/master/dist/blocks-source.js:142:13)
    at HTMLButtonElement.blocks.extend.on.ready.handler (https://cdn.rawgit.com/astoilkov/jsblocks/master/dist/blocks-source.js:7030:18)
    at HTMLButtonElement.<anonymous> (https://cdn.rawgit.com/astoilkov/jsblocks/master/dist/blocks-source.js:3861:18)
    4 selecting item: ITEM2
    4 item1._expressions.length is now: 3
    5 selecting item: ITEM1 1443190947498
    5 item1._expressions.length is now: 4
    6 selecting item: ITEM2
    6 item1._expressions.length is now: 4
    7 selecting item: ITEM1 1443190947498

@tomas2t
Copy link

tomas2t commented Sep 25, 2015

We realized this while trying to come up with a quick-n-dirty alternative render query (while waiting for #83).

Kanaye pushed a commit to Kanaye/jsblocks that referenced this issue Oct 3, 2015
Observables used in expressions will now get stored in
``elementData.observables`` before only data-query methods got stored
there.
Fixes astoilkov#90
astoilkov added a commit that referenced this issue Oct 4, 2015
Added garbadge collection for observables._expressions. FIxes #90
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants