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(ivy): remove query results from destroyed embedded views #28445

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 18 additions & 6 deletions packages/core/src/render3/node_manipulation.ts
Expand Up @@ -11,7 +11,7 @@ import {attachPatchData} from './context_discovery';
import {callHooks} from './hooks';
import {LContainer, NATIVE, VIEWS, unusedValueExportToPlacateAjd as unused1} from './interfaces/container';
import {ComponentDef} from './interfaces/definition';
import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeFlags, TNodeType, TProjectionNode, TViewNode, unusedValueExportToPlacateAjd as unused2} from './interfaces/node';
import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeFlags, TNodeType, TViewNode, unusedValueExportToPlacateAjd as unused2} from './interfaces/node';
import {unusedValueExportToPlacateAjd as unused3} from './interfaces/projection';
import {ProceduralRenderer3, RComment, RElement, RNode, RText, Renderer3, isProceduralRenderer, unusedValueExportToPlacateAjd as unused4} from './interfaces/renderer';
import {CLEANUP, CONTAINER_INDEX, FLAGS, HEADER_OFFSET, HOST_NODE, HookData, LView, LViewFlags, NEXT, PARENT, QUERIES, RENDERER, TVIEW, unusedValueExportToPlacateAjd as unused5} from './interfaces/view';
Expand Down Expand Up @@ -242,14 +242,17 @@ export function destroyViewTree(rootView: LView): void {
while (viewOrContainer) {
let next: LView|LContainer|null = null;

if (viewOrContainer.length >= HEADER_OFFSET) {
if (isLContainer(viewOrContainer)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Much clearer 👍

// If container, traverse down to its first LView.
const container = viewOrContainer as LContainer;
const viewsInContainer = container[VIEWS];
if (viewsInContainer.length) {
next = viewsInContainer[0];
}
} else {
// If LView, traverse down to child.
const view = viewOrContainer as LView;
if (view[TVIEW].childIndex > -1) next = getLViewChild(view);
} else {
// If container, traverse down to its first LView.
const container = viewOrContainer as LContainer;
if (container[VIEWS].length) next = container[VIEWS][0];
}

if (next == null) {
Expand All @@ -258,6 +261,15 @@ export function destroyViewTree(rootView: LView): void {
while (viewOrContainer && !viewOrContainer ![NEXT] && viewOrContainer !== rootView) {
cleanUpView(viewOrContainer);
viewOrContainer = getParentState(viewOrContainer, rootView);
if (isLContainer(viewOrContainer)) {
// this view will be destroyed so we need to notify queries that a view is detached
const viewsInContainer = (viewOrContainer as LContainer)[VIEWS];
for (let viewToDetach of viewsInContainer) {
if (viewToDetach[QUERIES]) {
viewToDetach[QUERIES] !.removeView();
}
}
}
}
cleanUpView(viewOrContainer || rootView);
next = viewOrContainer && viewOrContainer ![NEXT];
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/render3/util.ts
Expand Up @@ -127,7 +127,8 @@ export function isComponentDef<T>(def: DirectiveDef<T>): def is ComponentDef<T>
return (def as ComponentDef<T>).template !== null;
}

export function isLContainer(value: RElement | RComment | LContainer | StylingContext): boolean {
export function isLContainer(
value: RElement | RComment | LContainer | LView | StylingContext | null): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

We can let TypeScript narrow the type of value after this call, would avoid some casts :)

Suggested change
value: RElement | RComment | LContainer | LView | StylingContext | null): boolean {
value: RElement | RComment | LContainer | LView | StylingContext | null): value is LContainer {

// Styling contexts are also arrays, but their first index contains an element node
return Array.isArray(value) && value.length === LCONTAINER_LENGTH;
}
Expand Down
23 changes: 11 additions & 12 deletions packages/core/test/linker/query_integration_spec.ts
Expand Up @@ -635,25 +635,24 @@ describe('Query API', () => {
expect(q.query.map((d: TextDirective) => d.text)).toEqual(['2', '1']);
});

fixmeIvy('FW-920: Queries in nested views are not destroyed properly')
.it('should remove manually projected templates if their parent view is destroyed', () => {
const template = `
it('should remove manually projected templates if their parent view is destroyed', () => {
const template = `
<manual-projecting #q><ng-template #tpl><div text="1"></div></ng-template></manual-projecting>
<div *ngIf="shouldShow">
<ng-container [ngTemplateOutlet]="tpl"></ng-container>
</div>
`;
const view = createTestCmp(MyComp0, template);
const q = view.debugElement.children[0].references !['q'];
view.componentInstance.shouldShow = true;
view.detectChanges();
const view = createTestCmp(MyComp0, template);
const q = view.debugElement.children[0].references !['q'];
view.componentInstance.shouldShow = true;
view.detectChanges();

expect(q.query.length).toBe(1);
expect(q.query.length).toBe(1);

view.componentInstance.shouldShow = false;
view.detectChanges();
expect(q.query.length).toBe(0);
});
view.componentInstance.shouldShow = false;
view.detectChanges();
expect(q.query.length).toBe(0);
});

modifiedInIvy('https://github.com/angular/angular/issues/15117 fixed in ivy')
.it('should not throw if a content template is queried and created in the view during change detection',
Expand Down