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

fix(components) $dispose not always called #306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benouat
Copy link
Member

@benouat benouat commented Sep 24, 2014

Let's consider a template like that

<template test(data)>
  <#data.ref />
</template>

When changing the component template behinf data.ref corresponding
$dispose() method (if any defined in the controller class) were not
executed.

@benouat benouat force-pushed the components-lifecycle branch from fa83f40 to a4d9770 Compare September 24, 2014 15:48
benouat pushed a commit to benouat/hashspace that referenced this pull request Sep 24, 2014
Let's consider a template like that
```
<template test(data)>
  <#data.ref />
</template>
```

When changing the component template behinf `data.ref` corresponding
`$dispose()` method (if any defined in the controller class) were not
executed.

Closes ariatemplates#306
@benouat benouat force-pushed the components-lifecycle branch from a4d9770 to d245a52 Compare September 24, 2014 15:49
benouat pushed a commit to benouat/hashspace that referenced this pull request Sep 24, 2014
Let's consider a template like that
```
<template test(data)>
  <#data.ref />
</template>
```

When changing the component template behinf `data.ref` corresponding
`$dispose()` method (if any defined in the controller class) were not
executed.

Closes ariatemplates#306
@@ -600,11 +600,19 @@ var $CptNode = klass({
if (tplChanged) {
// check if component nature changed from template to component or opposite
this.template=tpl;
// Change might also be a different component ref, ie cpt1 -> cpt2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not enough as apparently the $dispose method is not called on the template root node (cf. $root.js line 120). So we should keep a reference on the template root node (which is returned when the template function is called) and call $dispose() on this reference. This should both dispose the node chain and the controller
(sorry I just realize this now!)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is somehow valid, but could be done in another PR.

Nevertheless, the $RootNode.$dispose method as far as I get it is only called when we don't want to keep localProperties on the node itself.

/**
 * Default dispose method
 * @param {Boolean} localPropOnly if true only local properties will be deleted (optional)
 *        must be used when a new instance is created to adapt to a path change
 */
$dispose:function(localPropOnly) {
    this.cleanObjectProperties(localPropOnly);
},

/**
 * Removes object properties - helper for $dispose methods
 * @param {Boolean} localPropOnly if true only local properties will be deleted (optional)
 *        must be used when a new instance is created to adapt to a path change
 */
cleanObjectProperties : function (localPropOnly) {
    this.removePathObservers();
    if (this._scopeChgeCb) {
        json.unobserve(this.vscope, this._scopeChgeCb);
        this._scopeChgeCb = null;
    }
    if (localPropOnly!==true) {
        $RootNode.$dispose.call(this);
    }
    this.exps = null;
    this.controller = null;
    this.ctlAttributes = null;
    this.template = null;
    if (this.node1) {
        this.node1=null;
        this.node2=null;
    }
}

@benouat benouat force-pushed the components-lifecycle branch from d245a52 to 95d5dcb Compare September 30, 2014 16:01
benouat pushed a commit to benouat/hashspace that referenced this pull request Sep 30, 2014
Let's consider a template like that
```
<template test(data)>
  <#data.ref />
</template>
```

When changing the component template behind `data.ref` corresponding
`$dispose()` method (if any defined in the controller class) was not
executed.

Closes ariatemplates#306
@benouat benouat force-pushed the components-lifecycle branch from 95d5dcb to 59b89c0 Compare October 1, 2014 09:57
benouat pushed a commit to benouat/hashspace that referenced this pull request Oct 1, 2014
Let's consider a template like that
```
<template test(data)>
  <#data.ref />
</template>
```

When changing the component template behind `data.ref` corresponding
`$dispose()` method (if any defined in the controller class) was not
executed.

Closes ariatemplates#306
@benouat benouat force-pushed the components-lifecycle branch from 59b89c0 to 7d44055 Compare October 1, 2014 15:24
benouat pushed a commit to benouat/hashspace that referenced this pull request Oct 1, 2014
Let's consider a template like that
```
<template test(data)>
  <#data.ref />
</template>
```

When changing the component template behind `data.ref` corresponding
`$dispose()` method (if any defined in the controller class) was not
executed.

Closes ariatemplates#306
@benouat benouat force-pushed the components-lifecycle branch from 7d44055 to f7c5c35 Compare October 1, 2014 15:34
benouat pushed a commit to benouat/hashspace that referenced this pull request Oct 1, 2014
Let's consider a template like that
```
<template test(data)>
  <#data.ref />
</template>
```

When changing the component template behind `data.ref` corresponding
`$dispose()` method (if any defined in the controller class) was not
executed.

Closes ariatemplates#306
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling f7c5c35 on benouat:components-lifecycle into d91db78 on ariatemplates:master.

Let's consider a template like that
```
<template test(data)>
  <#data.ref />
</template>
```

When changing the component template behind `data.ref` corresponding
`$dispose()` method (if any defined in the controller class) was not
executed.

Closes ariatemplates#306
@benouat benouat force-pushed the components-lifecycle branch from f7c5c35 to e751e80 Compare October 2, 2014 07:28
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 this pull request may close these issues.

3 participants