From 9fb541787c1d07ea208b7ecc55dd51afa647b568 Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Fri, 4 Sep 2020 13:03:46 -0700 Subject: [PATCH] refactor(core): Remove host `TNode` from `getOrCreateTNode` (#38707) Host `TNode` was passed into `getOrCreateTNode` just so that we can compute weather or not we are a root node. This was needed because `previousOrParentTNode` could have `TNode` from `TView` other then current `TView`. This is confusing mental model. Previous change ensured that `previousOrParentTNode` must always be part of `TView`, which enabled this change to remove the unneeded argument. PR Close #38707 --- packages/core/src/render3/component.ts | 2 +- packages/core/src/render3/di.ts | 22 ++++++------- packages/core/src/render3/i18n/i18n_apply.ts | 2 +- .../core/src/render3/instructions/element.ts | 2 +- .../render3/instructions/element_container.ts | 3 +- .../src/render3/instructions/projection.ts | 2 +- .../core/src/render3/instructions/shared.ts | 32 +++++++++---------- .../core/src/render3/instructions/template.ts | 2 +- .../core/src/render3/instructions/text.ts | 2 +- .../core/src/render3/node_manipulation.ts | 2 +- .../router/bundle.golden_symbols.json | 3 ++ packages/core/test/render3/di_spec.ts | 3 +- packages/core/test/render3/perf/setup.ts | 2 +- packages/core/test/render3/render_util.ts | 2 +- 14 files changed, 40 insertions(+), 41 deletions(-) diff --git a/packages/core/src/render3/component.ts b/packages/core/src/render3/component.ts index 693fe58629..ab01bea194 100644 --- a/packages/core/src/render3/component.ts +++ b/packages/core/src/render3/component.ts @@ -174,7 +174,7 @@ export function createRootComponentView( const tView = rootView[TVIEW]; ngDevMode && assertIndexInRange(rootView, 0 + HEADER_OFFSET); rootView[0 + HEADER_OFFSET] = rNode; - const tNode: TElementNode = getOrCreateTNode(tView, null, 0, TNodeType.Element, null, null); + const tNode: TElementNode = getOrCreateTNode(tView, 0, TNodeType.Element, null, null); const mergedAttrs = tNode.mergedAttrs = def.hostAttrs; if (mergedAttrs !== null) { computeStaticStyling(tNode, mergedAttrs, true); diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index 0c799cbcf2..b3b252f416 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -141,41 +141,41 @@ export function bloomAdd( * Creates (or gets an existing) injector for a given element or container. * * @param tNode for which an injector should be retrieved / created. - * @param hostView View where the node is stored + * @param lView View where the node is stored * @returns Node injector */ export function getOrCreateNodeInjectorForNode( - tNode: TElementNode|TContainerNode|TElementContainerNode, hostView: LView): number { - const existingInjectorIndex = getInjectorIndex(tNode, hostView); + tNode: TElementNode|TContainerNode|TElementContainerNode, lView: LView): number { + const existingInjectorIndex = getInjectorIndex(tNode, lView); if (existingInjectorIndex !== -1) { return existingInjectorIndex; } - const tView = hostView[TVIEW]; + const tView = lView[TVIEW]; if (tView.firstCreatePass) { - tNode.injectorIndex = hostView.length; + tNode.injectorIndex = lView.length; insertBloom(tView.data, tNode); // foundation for node bloom - insertBloom(hostView, null); // foundation for cumulative bloom + insertBloom(lView, null); // foundation for cumulative bloom insertBloom(tView.blueprint, null); } - const parentLoc = getParentInjectorLocation(tNode, hostView); + const parentLoc = getParentInjectorLocation(tNode, lView); const injectorIndex = tNode.injectorIndex; // If a parent injector can't be found, its location is set to -1. // In that case, we don't need to set up a cumulative bloom if (hasParentInjector(parentLoc)) { const parentIndex = getParentInjectorIndex(parentLoc); - const parentLView = getParentInjectorView(parentLoc, hostView); + const parentLView = getParentInjectorView(parentLoc, lView); const parentData = parentLView[TVIEW].data as any; // Creates a cumulative bloom filter that merges the parent's bloom filter // and its own cumulative bloom (which contains tokens for all ancestors) for (let i = 0; i < 8; i++) { - hostView[injectorIndex + i] = parentLView[parentIndex + i] | parentData[parentIndex + i]; + lView[injectorIndex + i] = parentLView[parentIndex + i] | parentData[parentIndex + i]; } } - hostView[injectorIndex + PARENT_INJECTOR] = parentLoc; + lView[injectorIndex + PARENT_INJECTOR] = parentLoc; return injectorIndex; } @@ -202,7 +202,7 @@ export function getInjectorIndex(tNode: TNode, hostView: LView): number { * Finds the index of the parent injector, with a view offset if applicable. Used to set the * parent injector initially. * - * Returns a combination of number of `ViewData` we have to go up and index in that `Viewdata` + * Returns a combination of number of `LView` we have to go up and index in that `LView` */ export function getParentInjectorLocation(tNode: TNode, view: LView): RelativeInjectorLocation { if (tNode.parent && tNode.parent.injectorIndex !== -1) { diff --git a/packages/core/src/render3/i18n/i18n_apply.ts b/packages/core/src/render3/i18n/i18n_apply.ts index ec73ac2cd1..5ef1b588bb 100644 --- a/packages/core/src/render3/i18n/i18n_apply.ts +++ b/packages/core/src/render3/i18n/i18n_apply.ts @@ -463,7 +463,7 @@ function createDynamicNodeAtIndex( ngDevMode && assertIndexInRange(lView, index + HEADER_OFFSET); lView[index + HEADER_OFFSET] = native; // FIXME(misko): Why does this create A TNode??? I would not expect this to be here. - const tNode = getOrCreateTNode(tView, lView[T_HOST], index, type as any, name, null); + const tNode = getOrCreateTNode(tView, index, type as any, name, null); // We are creating a dynamic node, the previous tNode might not be pointing at this node. // We will link ourselves into the tree later with `appendI18nNode`. diff --git a/packages/core/src/render3/instructions/element.ts b/packages/core/src/render3/instructions/element.ts index aaa2f8e4fe..8b12c8dde1 100644 --- a/packages/core/src/render3/instructions/element.ts +++ b/packages/core/src/render3/instructions/element.ts @@ -33,7 +33,7 @@ function elementStartFirstCreatePass( const tViewConsts = tView.consts; const attrs = getConstant(tViewConsts, attrsIndex); - const tNode = getOrCreateTNode(tView, lView[T_HOST], index, TNodeType.Element, name, attrs); + const tNode = getOrCreateTNode(tView, index, TNodeType.Element, name, attrs); const hasDirectives = resolveDirectives(tView, lView, tNode, getConstant(tViewConsts, localRefsIndex)); diff --git a/packages/core/src/render3/instructions/element_container.ts b/packages/core/src/render3/instructions/element_container.ts index 6a7a6ef7a4..f21b6c7d96 100644 --- a/packages/core/src/render3/instructions/element_container.ts +++ b/packages/core/src/render3/instructions/element_container.ts @@ -27,8 +27,7 @@ function elementContainerStartFirstCreatePass( const tViewConsts = tView.consts; const attrs = getConstant(tViewConsts, attrsIndex); - const tNode = getOrCreateTNode( - tView, lView[T_HOST], index, TNodeType.ElementContainer, 'ng-container', attrs); + const tNode = getOrCreateTNode(tView, index, TNodeType.ElementContainer, 'ng-container', attrs); // While ng-container doesn't necessarily support styling, we use the style context to identify // and execute directives on the ng-container. diff --git a/packages/core/src/render3/instructions/projection.ts b/packages/core/src/render3/instructions/projection.ts index 24219ba92b..edcfee126e 100644 --- a/packages/core/src/render3/instructions/projection.ts +++ b/packages/core/src/render3/instructions/projection.ts @@ -125,7 +125,7 @@ export function ɵɵprojection( const lView = getLView(); const tView = getTView(); const tProjectionNode = - getOrCreateTNode(tView, lView[T_HOST], nodeIndex, TNodeType.Projection, null, attrs || null); + getOrCreateTNode(tView, nodeIndex, TNodeType.Projection, null, attrs || null); // We can't use viewData[HOST_NODE] because projection nodes can be nested in embedded views. if (tProjectionNode.projection === null) tProjectionNode.projection = selectorIndex; diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index e998e0a0d4..837302c179 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -202,9 +202,6 @@ export function createLView( * Create and stores the TNode, and hooks it up to the tree. * * @param tView The current `TView`. - * @param tHostNode This is a hack and we should not have to pass this value in. It is only used to - * determine if the parent belongs to a different tView. Instead we should not have parentTView - * point to TView other the current one. * @param index The index at which the TNode should be saved (null if view, since they are not * saved). * @param type The type of TNode to create @@ -213,35 +210,34 @@ export function createLView( * @param attrs Any attrs for the native element, if applicable */ export function getOrCreateTNode( - tView: TView, tHostNode: TNode|null, index: number, type: TNodeType.Element, name: string|null, + tView: TView, index: number, type: TNodeType.Element, name: string|null, attrs: TAttributes|null): TElementNode; export function getOrCreateTNode( - tView: TView, tHostNode: TNode|null, index: number, type: TNodeType.Container, - name: string|null, attrs: TAttributes|null): TContainerNode; + tView: TView, index: number, type: TNodeType.Container, name: string|null, + attrs: TAttributes|null): TContainerNode; export function getOrCreateTNode( - tView: TView, tHostNode: TNode|null, index: number, type: TNodeType.Projection, name: null, + tView: TView, index: number, type: TNodeType.Projection, name: null, attrs: TAttributes|null): TProjectionNode; export function getOrCreateTNode( - tView: TView, tHostNode: TNode|null, index: number, type: TNodeType.ElementContainer, - name: string|null, attrs: TAttributes|null): TElementContainerNode; -export function getOrCreateTNode( - tView: TView, tHostNode: TNode|null, index: number, type: TNodeType.IcuContainer, name: null, + tView: TView, index: number, type: TNodeType.ElementContainer, name: string|null, attrs: TAttributes|null): TElementContainerNode; export function getOrCreateTNode( - tView: TView, tHostNode: TNode|null, index: number, type: TNodeType, name: string|null, - attrs: TAttributes|null): TElementNode&TContainerNode&TElementContainerNode&TProjectionNode& - TIcuContainerNode { + tView: TView, index: number, type: TNodeType.IcuContainer, name: null, + attrs: TAttributes|null): TElementContainerNode; +export function getOrCreateTNode( + tView: TView, index: number, type: TNodeType, name: string|null, attrs: TAttributes|null): + TElementNode&TContainerNode&TElementContainerNode&TProjectionNode&TIcuContainerNode { // Keep this function short, so that the VM will inline it. const adjustedIndex = index + HEADER_OFFSET; const tNode = tView.data[adjustedIndex] as TNode || - createTNodeAtIndex(tView, tHostNode, adjustedIndex, type, name, attrs); + createTNodeAtIndex(tView, adjustedIndex, type, name, attrs); setPreviousOrParentTNode(tNode, true); return tNode as TElementNode & TViewNode & TContainerNode & TElementContainerNode & TProjectionNode & TIcuContainerNode; } function createTNodeAtIndex( - tView: TView, tHostNode: TNode|null, adjustedIndex: number, type: TNodeType, name: string|null, + tView: TView, adjustedIndex: number, type: TNodeType, name: string|null, attrs: TAttributes|null) { const previousOrParentTNode = getPreviousOrParentTNode(); const isParent = getIsParent(); @@ -249,7 +245,9 @@ function createTNodeAtIndex( isParent ? previousOrParentTNode : previousOrParentTNode && previousOrParentTNode.parent; // Parents cannot cross component boundaries because components will be used in multiple places, // so it's only set if the view is the same. - const parentInSameView = parent && parent !== tHostNode; + // FIXME(misko): This check for `TNodeType.View` should not be needed. But removing it breaks DI, + // so more investigation is needed. + const parentInSameView = parent !== null && parent.type !== TNodeType.View; const tParentNode = parentInSameView ? parent as TElementNode | TContainerNode : null; const tNode = tView.data[adjustedIndex] = createTNode(tView, tParentNode, type, adjustedIndex, name, attrs); diff --git a/packages/core/src/render3/instructions/template.ts b/packages/core/src/render3/instructions/template.ts index 62195773ed..669b3c4a60 100644 --- a/packages/core/src/render3/instructions/template.ts +++ b/packages/core/src/render3/instructions/template.ts @@ -28,7 +28,7 @@ function templateFirstCreatePass( const tViewConsts = tView.consts; // TODO(pk): refactor getOrCreateTNode to have the "create" only version const tNode = getOrCreateTNode( - tView, lView[T_HOST], index, TNodeType.Container, tagName || null, + tView, index, TNodeType.Container, tagName || null, getConstant(tViewConsts, attrsIndex)); resolveDirectives(tView, lView, tNode, getConstant(tViewConsts, localRefsIndex)); diff --git a/packages/core/src/render3/instructions/text.ts b/packages/core/src/render3/instructions/text.ts index 88a27e8656..78bf0997ec 100644 --- a/packages/core/src/render3/instructions/text.ts +++ b/packages/core/src/render3/instructions/text.ts @@ -35,7 +35,7 @@ export function ɵɵtext(index: number, value: string = ''): void { ngDevMode && assertIndexInRange(lView, adjustedIndex); const tNode = tView.firstCreatePass ? - getOrCreateTNode(tView, lView[T_HOST], index, TNodeType.Element, null, null) : + getOrCreateTNode(tView, index, TNodeType.Element, null, null) : tView.data[adjustedIndex] as TElementNode; const textNative = lView[adjustedIndex] = createTextNode(value, lView[RENDERER]); diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 7f93eb07ec..ef5985973b 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -520,7 +520,7 @@ function getRenderParent(tView: TView, tNode: TNode, currentView: LView): REleme // can't be used as a render parent. let parentTNode = tNode.parent; while (parentTNode != null && - (parentTNode.type === TNodeType.ElementContainer || + (parentTNode.type === TNodeType.ElementContainer || parentTNode.type === TNodeType.View || parentTNode.type === TNodeType.IcuContainer)) { tNode = parentTNode; parentTNode = tNode.parent; diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 9b8b9aeba7..5eeba4b3f5 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -1355,6 +1355,9 @@ { "name": "getNodeInjectable" }, + { + "name": "getNonViewFirstChild" + }, { "name": "getNullInjector" }, diff --git a/packages/core/test/render3/di_spec.ts b/packages/core/test/render3/di_spec.ts index b80737ce5f..4343611162 100644 --- a/packages/core/test/render3/di_spec.ts +++ b/packages/core/test/render3/di_spec.ts @@ -230,8 +230,7 @@ describe('di', () => { LViewFlags.CheckAlways, null, null, {} as any, {} as any); enterView(contentView); try { - const parentTNode = - getOrCreateTNode(contentView[TVIEW], null, 0, TNodeType.Element, null, null); + const parentTNode = getOrCreateTNode(contentView[TVIEW], 0, TNodeType.Element, null, null); // Simulate the situation where the previous parent is not initialized. // This happens on first bootstrap because we don't init existing values // so that we have smaller HelloWorld. diff --git a/packages/core/test/render3/perf/setup.ts b/packages/core/test/render3/perf/setup.ts index 3e2e88b822..9ad6fd2a8e 100644 --- a/packages/core/test/render3/perf/setup.ts +++ b/packages/core/test/render3/perf/setup.ts @@ -50,7 +50,7 @@ export function setupTestHarness( directiveRegistry: DirectiveDefList|null = null): TestHarness { // Create a root view with a container const hostTView = createTView(TViewType.Root, -1, null, 1, 0, null, null, null, null, consts); - const tContainerNode = getOrCreateTNode(hostTView, null, 0, TNodeType.Container, null, null); + const tContainerNode = getOrCreateTNode(hostTView, 0, TNodeType.Container, null, null); const hostNode = renderer.createElement('div'); const hostLView = createLView( null, hostTView, {}, LViewFlags.CheckAlways | LViewFlags.IsRoot, hostNode, null, diff --git a/packages/core/test/render3/render_util.ts b/packages/core/test/render3/render_util.ts index 803e2bb8ae..342e790d7f 100644 --- a/packages/core/test/render3/render_util.ts +++ b/packages/core/test/render3/render_util.ts @@ -278,7 +278,7 @@ export function renderTemplate( def.pipeDefs = pipes || null; const componentTView = getOrCreateTComponentView(def); - const hostTNode = getOrCreateTNode(tView, hostLView[T_HOST], 0, TNodeType.Element, null, null); + const hostTNode = getOrCreateTNode(tView, 0, TNodeType.Element, null, null); hostLView[hostTNode.index] = hostNode; componentView = createLView( hostLView, componentTView, context, LViewFlags.CheckAlways, hostNode, hostTNode,