fix(template-outlet): memory leak for cached templates #15608
Conversation
When a TemplateOutlet instance was being destroyed all its locally cached views were not being destroyed. The parent component (grid) was attempting to clean up all the template outlets on the its destroy. But, this approach is weak because TemplateOutlets could have been destroyed along the way (resize, groupby, etc), so all those caches would have never been destroyed. The new strategy: * uniquely identified views (templateID.id) are meant to be managed by the parent component, so now the parent component destroys them directly and the template outlet never caches them locally. * other templates (local) are destroyed by the template outlet destroy.
pmoleri
added
❌ status: awaiting-test
|
There has been no recent activity and this PR has been marked inactive. |
github-actions
bot
added
the
status: inactive
ChronosSF
requested review from
MayaKirova,
damyanpetev and
igdmdimitrov
and removed request for
ChronosSF and
kdinev
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a memory leak in TemplateOutlet instances where cached views were not being properly destroyed when the template outlets themselves were destroyed. The fix implements a new strategy where uniquely identified views are managed by parent components while local templates are cleaned up by the template outlet's destroy lifecycle.
Key Changes:
- Implements proper view cleanup in ngOnDestroy for grid components and template outlets
- Updates caching strategy to differentiate between uniquely identified and local template views
- Adds templateID context for child row templates to enable proper parent management
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| template_outlet.directive.ts | Adds OnDestroy lifecycle and refined caching logic to prevent memory leaks |
| grid-base.directive.ts | Removes centralized template outlet cleanup approach |
| grid.component.ts | Adds OnDestroy implementation with child detail template cleanup |
| hierarchical-grid.component.ts | Adds child grid template cleanup and templateID context |
| row-island.component.ts | Updates grid state cleanup to destroy views directly |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const hasUniqueId = tmplID.id !== undefined && tmplID.id !== null; | ||
| if (hasUniqueId) { | ||
| // Don't cache locally unique id views, they're handled by the parent component by using moveview instead of cache |
There was a problem hiding this comment.
Consider using a more explicit check for unique IDs. The current logic treats tmplID.id = 0 or tmplID.id = false as non-unique, which may not be the intended behavior if these are valid ID values.
| const hasUniqueId = tmplID.id !== undefined && tmplID.id !== null; | |
| if (hasUniqueId) { | |
| // Don't cache locally unique id views, they're handled by the parent component by using moveview instead of cache | |
| // Don't cache views with a unique ID; they're handled by the parent component using moveView instead of cache | |
| if (tmplID.isUnique === true) { |
Copilot uses AI. Check for mistakes.
| this._embeddedViewsMap.set(tmplID.type, resCollection); | ||
| } | ||
|
|
||
| resCollection.set(tmplID.id, viewRef); |
There was a problem hiding this comment.
This line will always set undefined as the key since tmplID.id is guaranteed to be undefined or null at this point (line 197-201 filters out cases where it has a value). This suggests the caching logic may not work as intended for local templates.
Copilot uses AI. Check for mistakes.
3 participants
When a TemplateOutlet instance was being destroyed all its locally cached views were not being destroyed.
The parent component (grid) was attempting to clean up all the template outlets on the its destroy. But, this approach is weak because TemplateOutlets could have been destroyed along the way (resize, groupby, etc), so all those caches would have never been destroyed.
The new strategy:
Example bug scenario in the old code:
Repro:
<igx-grid-row>elementsNote that this repro is based on 19.1.5 which doesn't contain other fixes. So, you'll find also other leaks.
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)