Skip to content

Commit 2fc0fbe

Browse files
JeanMecheJeanMeche
committed
fix(core): ensure proper cleanup of transplanted views
This fixes two issues: 1. Prevents double-detachment bugs where detachView performs redundant DOM manipulation and query detachments on a view that is already destroyed. 2. Ensures the LContainerFlags.HasTransplantedViews flag is cleared when the last transplanted view is detached, preventing unnecessary change detection traversals.
1 parent 846c73d commit 2fc0fbe

3 files changed

Lines changed: 133 additions & 10 deletions

File tree

‎packages/core/src/render3/node_manipulation.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,13 @@ export function detachMovedView(declarationContainer: LContainer, lView: LView)
305305
);
306306
const movedViews = declarationContainer[MOVED_VIEWS]!;
307307
const declarationViewIndex = movedViews.indexOf(lView);
308-
movedViews.splice(declarationViewIndex, 1);
308+
if (declarationViewIndex !== -1) {
309+
movedViews.splice(declarationViewIndex, 1);
310+
if (movedViews.length === 0) {
311+
declarationContainer[MOVED_VIEWS] = null;
312+
declarationContainer[FLAGS] &= ~LContainerFlags.HasTransplantedViews;
313+
}
314+
}
309315
}
310316

311317
/**

‎packages/core/src/render3/view/container.ts‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import {
1818
} from '../interfaces/container';
1919
import {TNode} from '../interfaces/node';
2020
import {RComment, RElement} from '../interfaces/renderer_dom';
21-
import {isLView} from '../interfaces/type_checks';
21+
import {isDestroyed, isLView} from '../interfaces/type_checks';
2222
import {
2323
DECLARATION_COMPONENT_VIEW,
2424
DECLARATION_LCONTAINER,
@@ -153,21 +153,28 @@ export function detachView(lContainer: LContainer, removeIndex: number): LView |
153153
const viewToDetach = lContainer[indexInContainer];
154154

155155
if (viewToDetach) {
156-
const declarationLContainer = viewToDetach[DECLARATION_LCONTAINER];
157-
if (declarationLContainer !== null && declarationLContainer !== lContainer) {
158-
detachMovedView(declarationLContainer, viewToDetach);
156+
const isViewDestroyed = isDestroyed(viewToDetach);
157+
158+
if (!isViewDestroyed) {
159+
const declarationLContainer = viewToDetach[DECLARATION_LCONTAINER];
160+
if (declarationLContainer !== null && declarationLContainer !== lContainer) {
161+
detachMovedView(declarationLContainer, viewToDetach);
162+
}
159163
}
160164

161165
if (removeIndex > 0) {
162166
lContainer[indexInContainer - 1][NEXT] = viewToDetach[NEXT] as LView;
163167
}
164168
const removedLView = removeFromArray(lContainer, CONTAINER_HEADER_OFFSET + removeIndex);
165-
removeViewFromDOM(viewToDetach[TVIEW], viewToDetach);
166169

167-
// notify query that a view has been removed
168-
const lQueries = removedLView[QUERIES];
169-
if (lQueries !== null) {
170-
lQueries.detachView(removedLView[TVIEW]);
170+
if (!isViewDestroyed) {
171+
removeViewFromDOM(viewToDetach[TVIEW], viewToDetach);
172+
173+
// notify query that a view has been removed
174+
const lQueries = removedLView[QUERIES];
175+
if (lQueries !== null) {
176+
lQueries.detachView(removedLView[TVIEW]);
177+
}
171178
}
172179

173180
viewToDetach[PARENT] = null;

‎packages/core/test/acceptance/change_detection_transplanted_view_spec.ts‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ import {
3030
ViewContainerRef,
3131
} from '../../src/core';
3232
import {provideCheckNoChangesConfig} from '../../src/change_detection/provide_check_no_changes_config';
33+
import {LContainerFlags, MOVED_VIEWS} from '../../src/render3/interfaces/container';
34+
import {FLAGS, LView, QUERIES} from '../../src/render3/interfaces/view';
3335
import {ComponentFixture, TestBed} from '../../testing';
3436
import {expect} from '@angular/private/testing/matchers';
3537
import {timeout} from '@angular/private/testing';
@@ -756,6 +758,114 @@ describe('change detection for transplanted views', () => {
756758
);
757759
});
758760

761+
it('keeps refreshing other transplanted views when an insertion container is cleared on destroy', () => {
762+
@Component({
763+
selector: 'outlet',
764+
template: '<ng-container #container />',
765+
standalone: true,
766+
changeDetection: ChangeDetectionStrategy.OnPush,
767+
})
768+
class Outlet {
769+
@Input() template!: TemplateRef<{}>;
770+
@ViewChild('container', {read: ViewContainerRef, static: true})
771+
container!: ViewContainerRef;
772+
773+
ngOnInit() {
774+
this.container.createEmbeddedView(this.template);
775+
}
776+
777+
ngOnDestroy() {
778+
this.container.clear();
779+
}
780+
}
781+
782+
@Component({
783+
template: `
784+
<ng-template #template>{{ name }}</ng-template>
785+
@for (item of items; track item) {
786+
<outlet [template]="template"></outlet>
787+
}
788+
`,
789+
standalone: true,
790+
imports: [Outlet],
791+
changeDetection: ChangeDetectionStrategy.Eager,
792+
})
793+
class App {
794+
items = [1, 2, 3];
795+
name = 'Penny';
796+
}
797+
798+
const fixture = TestBed.createComponent(App);
799+
fixture.detectChanges();
800+
expect(fixture.nativeElement.textContent).toEqual('PennyPennyPenny');
801+
802+
fixture.componentInstance.items = [1, 2];
803+
fixture.detectChanges();
804+
fixture.componentInstance.name = 'Sheldon';
805+
fixture.detectChanges();
806+
expect(fixture.nativeElement.textContent).toEqual('SheldonSheldon');
807+
});
808+
809+
it('should not double-detach queries and should clear HasTransplantedViews flag', () => {
810+
@Component({
811+
selector: 'outlet',
812+
template: '<ng-container #container />',
813+
standalone: true,
814+
})
815+
class Outlet {
816+
@Input() template!: TemplateRef<{}>;
817+
@ViewChild('container', {read: ViewContainerRef, static: true})
818+
container!: ViewContainerRef;
819+
820+
ngOnInit() {
821+
this.container.createEmbeddedView(this.template);
822+
}
823+
824+
ngOnDestroy() {
825+
this.container.clear();
826+
}
827+
}
828+
829+
@Component({
830+
template: `
831+
<!-- We use a query to force the instantiation of LQueries -->
832+
<ng-template #template><div #myQuery></div></ng-template>
833+
@if (show) {
834+
<outlet [template]="template"></outlet>
835+
}
836+
`,
837+
standalone: true,
838+
imports: [Outlet],
839+
})
840+
class App {
841+
@ViewChild('template', {read: TemplateRef, static: true})
842+
template!: TemplateRef<any>;
843+
show = true;
844+
}
845+
846+
const fixture = TestBed.createComponent(App);
847+
fixture.detectChanges();
848+
849+
const appLView = (fixture.componentInstance as any).__ngContext__ as LView;
850+
const declarationContainer = appLView[20] as any;
851+
const transplantedLView = declarationContainer[MOVED_VIEWS]![0] as LView;
852+
const lQueries = transplantedLView[QUERIES]!;
853+
854+
const detachViewSpy = spyOn(lQueries, 'detachView').and.callThrough();
855+
856+
fixture.componentInstance.show = false;
857+
fixture.detectChanges();
858+
859+
expect(detachViewSpy).toHaveBeenCalledTimes(1);
860+
861+
const hasTransplantedViewsFlag =
862+
(declarationContainer[FLAGS] & LContainerFlags.HasTransplantedViews) ===
863+
LContainerFlags.HasTransplantedViews;
864+
865+
expect(declarationContainer[MOVED_VIEWS]).toBeNull();
866+
expect(hasTransplantedViewsFlag).toBeFalse();
867+
});
868+
759869
describe('ViewRef and ViewContainerRef operations', () => {
760870
@Component({
761871
template: '<ng-template>{{incrementChecks()}}</ng-template>',

0 commit comments

Comments
 (0)