From b5c2ef287786ab86db7020656a08921a093bcb96 Mon Sep 17 00:00:00 2001 From: JoostK Date: Wed, 2 Jan 2019 19:29:08 +0100 Subject: [PATCH] fix(ivy): clone queries correctly for multiple component instances (#27892) When requesting a queries instance for a node, it was previously decided whether it needs to be cloned if the node was not already marked as hosting a query. This check is in place to have only a single queries instance per node. The issue with this approach is that no clone is created for subsequent instantiations of a component, as the TNode is already marked as hosting a query during first template pass, whereas the cloning of queries should be independent of first template pass. To overcome this issue, the queries are assigned an owner TNode such that it can reliably be determined if a clone needs to be created. PR Close #27892 --- packages/core/src/render3/instructions.ts | 19 ++++-- packages/core/src/render3/state.ts | 5 +- .../bundling/todo/bundle.golden_symbols.json | 3 + packages/core/test/render3/query_spec.ts | 58 +++++++++++++++++++ 4 files changed, 77 insertions(+), 8 deletions(-) diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index ca4929b23e..fb895bfd3a 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -41,7 +41,7 @@ import {getInitialClassNameValue, initializeStaticContext as initializeStaticSty import {BoundPlayerFactory} from './styling/player_factory'; import {createEmptyStylingContext, getStylingContext, hasClassInput, hasStyling, isAnimationProp} from './styling/util'; import {NO_CHANGE} from './tokens'; -import {findComponentView, getComponentViewByIndex, getNativeByIndex, getNativeByTNode, getRootContext, getRootView, getTNode, isComponent, isComponentDef, loadInternal, readElementValue, readPatchedLView, renderStringify} from './util'; +import {findComponentView, getComponentViewByIndex, getNativeByIndex, getNativeByTNode, getRootContext, getRootView, getTNode, isComponent, isComponentDef, isContentQueryHost, loadInternal, readElementValue, readPatchedLView, renderStringify} from './util'; @@ -624,7 +624,7 @@ export function elementCreate(name: string, overriddenRenderer?: Renderer3): REl * @param localRefExtractor mapping function that extracts local ref value from TNode */ function createDirectivesAndLocals( - tView: TView, viewData: LView, localRefs: string[] | null | undefined, + tView: TView, lView: LView, localRefs: string[] | null | undefined, localRefExtractor: LocalRefExtractor = getNativeByTNode) { if (!getBindingsEnabled()) return; const previousOrParentTNode = getPreviousOrParentTNode(); @@ -632,12 +632,19 @@ function createDirectivesAndLocals( ngDevMode && ngDevMode.firstTemplatePass++; resolveDirectives( - tView, viewData, findDirectiveMatches(tView, viewData, previousOrParentTNode), + tView, lView, findDirectiveMatches(tView, lView, previousOrParentTNode), previousOrParentTNode, localRefs || null); + } else { + // During first template pass, queries are created or cloned when first requested + // using `getOrCreateCurrentQueries`. For subsequent template passes, we clone + // any current LQueries here up-front if the current node hosts a content query. + if (isContentQueryHost(getPreviousOrParentTNode()) && lView[QUERIES]) { + lView[QUERIES] = lView[QUERIES] !.clone(); + } } - instantiateAllDirectives(tView, viewData, previousOrParentTNode); - invokeDirectivesHostBindings(tView, viewData, previousOrParentTNode); - saveResolvedLocalsInData(viewData, previousOrParentTNode, localRefExtractor); + instantiateAllDirectives(tView, lView, previousOrParentTNode); + invokeDirectivesHostBindings(tView, lView, previousOrParentTNode); + saveResolvedLocalsInData(lView, previousOrParentTNode, localRefExtractor); } /** diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts index 123208d331..b259f70035 100644 --- a/packages/core/src/render3/state.ts +++ b/packages/core/src/render3/state.ts @@ -175,8 +175,9 @@ export function getOrCreateCurrentQueries( QueryType: {new (parent: null, shallow: null, deep: null): LQueries}): LQueries { const lView = getLView(); let currentQueries = lView[QUERIES]; - // if this is the first content query on a node, any existing LQueries needs to be cloned - // in subsequent template passes, the cloning occurs before directive instantiation. + // If this is the first content query on a node, any existing LQueries needs to be cloned. + // In subsequent template passes, the cloning occurs before directive instantiation + // in `createDirectivesAndLocals`. if (previousOrParentTNode && previousOrParentTNode !== lView[HOST_NODE] && !isContentQueryHost(previousOrParentTNode)) { currentQueries && (currentQueries = lView[QUERIES] = currentQueries.clone()); diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index b4c3aabbfb..3fa175255b 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -878,6 +878,9 @@ { "name": "isComponentDef" }, + { + "name": "isContentQueryHost" + }, { "name": "isContextDirty" }, diff --git a/packages/core/test/render3/query_spec.ts b/packages/core/test/render3/query_spec.ts index d5b571c795..4dca778d71 100644 --- a/packages/core/test/render3/query_spec.ts +++ b/packages/core/test/render3/query_spec.ts @@ -2604,6 +2604,64 @@ describe('query', () => { expect(inInstance !.fooBars.length).toBe(2); }); + it('should support nested shallow content queries across multiple component instances', () => { + let outInstance: QueryDirective; + let inInstance: QueryDirective; + + class QueryDirective { + fooBars: any; + static ngDirectiveDef = defineDirective({ + type: QueryDirective, + selectors: [['', 'query', '']], + exportAs: ['query'], + factory: () => new QueryDirective(), + contentQueries: (dirIndex) => { + // @ContentChildren('foo', {descendants: true}) fooBars: + // QueryList; + registerContentQuery(query(null, ['foo'], false), dirIndex); + }, + contentQueriesRefresh: (dirIndex: number, queryStartIdx: number) => { + let tmp: any; + const instance = load(dirIndex); + queryRefresh(tmp = loadQueryList(queryStartIdx)) && + (instance.fooBars = tmp); + }, + }); + } + + const AppComponent = createComponent( + 'app-component', + /** + *
+ *
+ * + *
+ *
+ */ + function(rf: RenderFlags, ctx: any) { + if (rf & RenderFlags.Create) { + elementStart(0, 'div', ['query', ''], ['out', 'query']); + { element(2, 'div', ['query', ''], ['in', 'query', 'foo', '']); } + elementEnd(); + } + if (rf & RenderFlags.Update) { + outInstance = load(1); + inInstance = load(3); + } + }, + 5, 0, [QueryDirective]); + + const fixture1 = new ComponentFixture(AppComponent); + expect(outInstance !.fooBars.length).toBe(1); + expect(inInstance !.fooBars.length).toBe(1); + + outInstance = inInstance = null !; + + const fixture2 = new ComponentFixture(AppComponent); + expect(outInstance !.fooBars.length).toBe(1); + expect(inInstance !.fooBars.length).toBe(1); + }); + it('should respect shallow flag on content queries when mixing deep and shallow queries', () => { class ShallowQueryDirective {