[BUGFIX release] don’t leak last destroyedComponents #14987
Conversation
Ensure we don’t retain the previous set of destroyed components.
| // components queued for destruction must be destroyed before firing | ||
| // `didCreate` to prevent errors when removing and adding a component | ||
| // with the same name (would throw an error when added to view registry) | ||
| for (let i = 0; i < this.destroyedComponents.length; i++) { | ||
| this.destroyedComponents[i].destroy(); | ||
| for (let i = 0; i < destroyedComponents.length; i++) { |
There was a problem hiding this comment.
Don't you want to cache length here?
There was a problem hiding this comment.
lol stop messing with the old guy...
| } | ||
|
|
||
| commit() { | ||
| let destroyedComponents = this.destroyedComponents; | ||
| this.destroyedComponents = []; |
There was a problem hiding this comment.
Why do this assignment here. Why not just reassign to an empty array after the subsequent loop? More of a stylistic thing I think.
There was a problem hiding this comment.
if the loop fails (destroy could throw), we still want to release the contents of the original destroyedComponents. So this way, we ensure they are released before we invoke any further code.
|
|
||
| this.registerComponent('foo-bar', { ComponentClass: FooBarComponent, template: 'hello' }); | ||
|
|
||
| this.render('{{#if switch}}{{#foo-bar}}{{foo-bar}}{{/foo-bar}}{{/if}}', { switch: true }); |
There was a problem hiding this comment.
|
Can you set the length to 0? It will release it, is there a reason to make a new array? |
I could, in this case I didn't believe the cost of a new array mattered so i left it closer to how it was originally implemented. |
|
Good catch @stefanpenner! |
Ensure we don’t retain the previous set of destroyed components.
Although
commitdid reset thedestroyedComponentsarray, it does seem strange to retain the last set ofdestroyedComponentsuntil the next render.