From 812615bb997f18f99becad0851d7412c11bf2030 Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Thu, 3 Sep 2020 22:46:11 -0700 Subject: [PATCH] refactor(core): Ensure that `previousOrParentTNode` always belongs to current `TView`. (#38707) `previousOrParentTNode` stores current `TNode`. Due to inconsistent implementation the value stored would sometimes belong to the current `TView` and sometimes to the parent. We have extra logic which accounts for it. A better solution is to just ensure that `previousOrParentTNode` always belongs to current `TNode`. This simplifies the mental model and cleans up some code. PR Close #38707 --- goldens/size-tracking/integration-payloads.json | 2 +- packages/core/src/render3/assert.ts | 9 +++++++-- packages/core/src/render3/component.ts | 2 +- packages/core/src/render3/component_ref.ts | 2 +- packages/core/src/render3/i18n/i18n_apply.ts | 2 +- packages/core/src/render3/instructions/shared.ts | 6 +++--- .../core/src/render3/instructions/template.ts | 6 ++++-- packages/core/src/render3/node_manipulation.ts | 8 ++++---- packages/core/src/render3/state.ts | 10 ++++++---- packages/core/src/render3/util/view_utils.ts | 15 ++++++++++++++- packages/core/src/render3/view_ref.ts | 4 ++-- .../bundling/forms/bundle.golden_symbols.json | 3 +++ .../test/bundling/todo/bundle.golden_symbols.json | 3 +++ packages/core/test/render3/di_spec.ts | 2 +- packages/core/test/render3/i18n_spec.ts | 6 ++++++ .../test/render3/instructions/lview_debug_spec.ts | 2 +- .../core/test/render3/instructions/shared_spec.ts | 2 +- packages/core/test/render3/render_util.ts | 2 +- .../render3/styling_next/static_styling_spec.ts | 2 +- .../styling_next/style_binding_list_spec.ts | 2 +- .../core/test/sanitization/sanitization_spec.ts | 2 +- 21 files changed, 63 insertions(+), 29 deletions(-) diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index a0232300df..63282d2d22 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/goldens/size-tracking/integration-payloads.json @@ -3,7 +3,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 141151, + "main-es2015": 141711, "polyfills-es2015": 36571 } } diff --git a/packages/core/src/render3/assert.ts b/packages/core/src/render3/assert.ts index ab82f9a85a..a48c924e19 100644 --- a/packages/core/src/render3/assert.ts +++ b/packages/core/src/render3/assert.ts @@ -20,10 +20,15 @@ import {LView, TVIEW, TView} from './interfaces/view'; export function assertTNodeForLView(tNode: TNode, lView: LView) { + assertTNodeForTView(tNode, lView[TVIEW]); +} + +export function assertTNodeForTView(tNode: TNode, tView: TView) { + assertDefined(tNode, 'TNode must be defined'); tNode.hasOwnProperty('tView_') && assertEqual( - (tNode as any as {tView_: TView}).tView_, lView[TVIEW], - 'This TNode does not belong to this LView.'); + (tNode as any as {tView_: TView}).tView_, tView, + 'This TNode does not belong to this TView.'); } export function assertComponentType( diff --git a/packages/core/src/render3/component.ts b/packages/core/src/render3/component.ts index 0f411079a1..693fe58629 100644 --- a/packages/core/src/render3/component.ts +++ b/packages/core/src/render3/component.ts @@ -134,7 +134,7 @@ export function renderComponent( null, rootTView, rootContext, rootFlags, null, null, rendererFactory, renderer, undefined, opts.injector || null); - enterView(rootView, null); + enterView(rootView); let component: T; try { diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts index eed3fb24d6..aa2b37a26e 100644 --- a/packages/core/src/render3/component_ref.ts +++ b/packages/core/src/render3/component_ref.ts @@ -168,7 +168,7 @@ export class ComponentFactory extends viewEngine_ComponentFactory { // `renderView` does that. However as the code is written it is needed because // `createRootComponentView` and `createRootComponent` both read global state. Fixing those // issues would allow us to drop this. - enterView(rootLView, null); + enterView(rootLView); let component: T; let tElementNode: TElementNode; diff --git a/packages/core/src/render3/i18n/i18n_apply.ts b/packages/core/src/render3/i18n/i18n_apply.ts index af7728decd..ec73ac2cd1 100644 --- a/packages/core/src/render3/i18n/i18n_apply.ts +++ b/packages/core/src/render3/i18n/i18n_apply.ts @@ -412,7 +412,7 @@ export function i18nEndFirstPass(tView: TView, lView: LView) { // Remove deleted nodes let index = rootIndex + 1; - while (index <= lastCreatedNode.index - HEADER_OFFSET) { + while (lastCreatedNode !== null && index <= lastCreatedNode.index - HEADER_OFFSET) { if (visitedNodes.indexOf(index) === -1) { removeNode(tView, lView, index, /* markAsDetached */ true); } diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 5f1e6be5e0..e998e0a0d4 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -337,7 +337,7 @@ export function allocExpando(tView: TView, lView: LView, numSlotsToAlloc: number */ export function renderView(tView: TView, lView: LView, context: T): void { ngDevMode && assertEqual(isCreationMode(lView), true, 'Should be run in creation mode'); - enterView(lView, lView[T_HOST]); + enterView(lView); try { const viewQuery = tView.viewQuery; if (viewQuery !== null) { @@ -407,7 +407,7 @@ export function refreshView( ngDevMode && assertEqual(isCreationMode(lView), false, 'Should be run in update mode'); const flags = lView[FLAGS]; if ((flags & LViewFlags.Destroyed) === LViewFlags.Destroyed) return; - enterView(lView, lView[T_HOST]); + enterView(lView); const checkNoChangesMode = getCheckNoChangesMode(); try { resetPreOrderHookFlags(lView); @@ -623,7 +623,7 @@ export function getOrCreateTComponentView(def: ComponentDef): TView { const tView = def.tView; // Create a TView if there isn't one, or recreate it if the first create pass didn't - // complete successfuly since we can't know for sure whether it's in a usable shape. + // complete successfully since we can't know for sure whether it's in a usable shape. if (tView === null || tView.incompleteFirstPass) { return def.tView = createTView( TViewType.Component, -1, def.template, def.decls, def.vars, def.directiveDefs, diff --git a/packages/core/src/render3/instructions/template.ts b/packages/core/src/render3/instructions/template.ts index a21594ae62..62195773ed 100644 --- a/packages/core/src/render3/instructions/template.ts +++ b/packages/core/src/render3/instructions/template.ts @@ -37,9 +37,11 @@ function templateFirstCreatePass( const embeddedTView = tNode.tViews = createTView( TViewType.Embedded, -1, templateFn, decls, vars, tView.directiveRegistry, tView.pipeRegistry, null, tView.schemas, tViewConsts); - const embeddedTViewNode = createTNode(tView, null, TNodeType.View, -1, null, null) as TViewNode; + const embeddedTViewNode = + createTNode(embeddedTView, null, TNodeType.View, -1, null, null) as TViewNode; embeddedTViewNode.injectorIndex = tNode.injectorIndex; - embeddedTView.node = embeddedTViewNode; + // FIXME(misko): remove `embeddedTView.node' + embeddedTView.node = embeddedTView.firstChild = embeddedTViewNode; if (tView.queries !== null) { tView.queries.template(tView, tNode); diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 8903073181..7f93eb07ec 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -9,7 +9,7 @@ import {Renderer2} from '../core'; import {ViewEncapsulation} from '../metadata/view'; import {addToArray, removeFromArray} from '../util/array_utils'; -import {assertDefined, assertDomNode, assertEqual, assertString} from '../util/assert'; +import {assertDefined, assertDomNode, assertEqual, assertSame, assertString} from '../util/assert'; import {assertLContainer, assertLView, assertTNodeForLView} from './assert'; import {attachPatchData} from './context_discovery'; @@ -23,7 +23,7 @@ import {isLContainer, isLView} from './interfaces/type_checks'; import {CHILD_HEAD, CLEANUP, DECLARATION_COMPONENT_VIEW, DECLARATION_LCONTAINER, DestroyHookData, FLAGS, HookData, HookFn, HOST, LView, LViewFlags, NEXT, PARENT, QUERIES, RENDERER, T_HOST, TVIEW, TView, unusedValueExportToPlacateAjd as unused5} from './interfaces/view'; import {assertNodeOfPossibleTypes, assertNodeType} from './node_assert'; import {getLViewParent} from './util/view_traversal_utils'; -import {getNativeByTNode, unwrapRNode, updateTransplantedViewCount} from './util/view_utils'; +import {getNativeByTNode, getNonViewFirstChild, unwrapRNode, updateTransplantedViewCount} from './util/view_utils'; const unusedValueToPlacateAjd = unused1 + unused2 + unused3 + unused4 + unused5; @@ -733,7 +733,7 @@ export function getBeforeNodeForView(viewIndexInContainer: number, lContainer: L const nextViewIndex = CONTAINER_HEADER_OFFSET + viewIndexInContainer + 1; if (nextViewIndex < lContainer.length) { const lView = lContainer[nextViewIndex] as LView; - const firstTNodeOfView = lView[TVIEW].firstChild; + const firstTNodeOfView = getNonViewFirstChild(lView[TVIEW]); if (firstTNodeOfView !== null) { return getFirstNativeNode(lView, firstTNodeOfView); } @@ -824,7 +824,7 @@ function applyView( tView: TView, lView: LView, renderer: Renderer3, action: WalkTNodeTreeAction, renderParent: RElement|null, beforeNode: RNode|null) { ngDevMode && assertNodeType(tView.node!, TNodeType.View); - const viewRootTNode: TNode|null = tView.node!.child; + const viewRootTNode: TNode|null = getNonViewFirstChild(tView); applyNodes(renderer, action, viewRootTNode, lView, renderParent, beforeNode, false); } diff --git a/packages/core/src/render3/state.ts b/packages/core/src/render3/state.ts index e0e70ef7d7..5ef6bc0c88 100644 --- a/packages/core/src/render3/state.ts +++ b/packages/core/src/render3/state.ts @@ -7,7 +7,7 @@ */ import {assertDefined, assertEqual} from '../util/assert'; -import {assertLViewOrUndefined} from './assert'; +import {assertLViewOrUndefined, assertTNodeForTView} from './assert'; import {DirectiveDef} from './interfaces/definition'; import {TNode} from './interfaces/node'; import {CONTEXT, DECLARATION_VIEW, LView, OpaqueViewState, TData, TVIEW, TView} from './interfaces/view'; @@ -54,6 +54,7 @@ interface LFrame { * * This is used in conjunction with `isParent`. */ + // FIXME(misko): should be `TNode|null` (add comment explaining it.) previousOrParentTNode: TNode; /** @@ -267,6 +268,7 @@ export function getPreviousOrParentTNode(): TNode { } export function setPreviousOrParentTNode(tNode: TNode, isParent: boolean) { + ngDevMode && assertTNodeForTView(tNode, instructionState.lFrame.tView); instructionState.lFrame.previousOrParentTNode = tNode; instructionState.lFrame.isParent = isParent; } @@ -401,10 +403,9 @@ export function enterDI(newView: LView, tNode: TNode) { * exited the state has to be restored * * @param newView New lView to become active - * @param tNode Element to which the View is a child of * @returns the previously active lView; */ -export function enterView(newView: LView, tNode: TNode|null): void { +export function enterView(newView: LView): void { ngDevMode && assertLViewOrUndefined(newView); const newLFrame = allocLFrame(); if (ngDevMode) { @@ -420,7 +421,8 @@ export function enterView(newView: LView, tNode: TNode|null): void { } const tView = newView[TVIEW]; instructionState.lFrame = newLFrame; - newLFrame.previousOrParentTNode = tNode!; + ngDevMode && tView.firstChild && assertTNodeForTView(tView.firstChild, tView); + newLFrame.previousOrParentTNode = tView.firstChild!; newLFrame.lView = newView; newLFrame.tView = tView; newLFrame.contextLView = newView!; diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts index 4876a8a7d1..6ead6bb400 100644 --- a/packages/core/src/render3/util/view_utils.ts +++ b/packages/core/src/render3/util/view_utils.ts @@ -10,7 +10,7 @@ import {assertDefined, assertDomNode, assertGreaterThan, assertIndexInRange, ass import {assertTNodeForLView} from '../assert'; import {LContainer, TYPE} from '../interfaces/container'; import {LContext, MONKEY_PATCH_KEY_NAME} from '../interfaces/context'; -import {TConstants, TNode} from '../interfaces/node'; +import {TConstants, TNode, TNodeType} from '../interfaces/node'; import {isProceduralRenderer, RNode} from '../interfaces/renderer'; import {isLContainer, isLView} from '../interfaces/type_checks'; import {FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, PARENT, PREORDER_HOOK_FLAGS, RENDERER, TData, TRANSPLANTED_VIEWS_TO_REFRESH, TView} from '../interfaces/view'; @@ -207,3 +207,16 @@ export function updateTransplantedViewCount(lContainer: LContainer, amount: 1|- parent = parent[PARENT]; } } + +/** + * Retrieves the `TView.firstChild` and unwraps if it is `TNodeType.View`. + * + * We are inconsistent about the way we store root of Views. Embedded views have `TNodeType.View` in + * the root but component views do not. A lot of logic does not expect to see `TNodeType.View` and + * crashes on it, so we unwrap it. + */ +export function getNonViewFirstChild(tView: TView): TNode|null { + const firstChild = tView.firstChild; + return firstChild === null ? null : + (firstChild.type === TNodeType.View ? firstChild.child : firstChild); +} diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index f4df71d91d..6641177f9f 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -19,7 +19,7 @@ import {CONTEXT, DECLARATION_COMPONENT_VIEW, FLAGS, HOST, LView, LViewFlags, T_H import {assertNodeOfPossibleTypes} from './node_assert'; import {destroyLView, renderDetachView} from './node_manipulation'; import {getLViewParent} from './util/view_traversal_utils'; -import {unwrapRNode} from './util/view_utils'; +import {getNonViewFirstChild, unwrapRNode} from './util/view_utils'; @@ -340,7 +340,7 @@ function collectNativeNodes( if (isLContainer(lNode)) { for (let i = CONTAINER_HEADER_OFFSET; i < lNode.length; i++) { const lViewInAContainer = lNode[i]; - const lViewFirstChildTNode = lViewInAContainer[TVIEW].firstChild; + const lViewFirstChildTNode = getNonViewFirstChild(lViewInAContainer[TVIEW]); if (lViewFirstChildTNode !== null) { collectNativeNodes( lViewInAContainer[TVIEW], lViewInAContainer, lViewFirstChildTNode, result); diff --git a/packages/core/test/bundling/forms/bundle.golden_symbols.json b/packages/core/test/bundling/forms/bundle.golden_symbols.json index 116250dea7..68600167d0 100644 --- a/packages/core/test/bundling/forms/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms/bundle.golden_symbols.json @@ -1031,6 +1031,9 @@ { "name": "getNodeInjectable" }, + { + "name": "getNonViewFirstChild" + }, { "name": "getNullInjector" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index bac2ef604c..96dee27a67 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -395,6 +395,9 @@ { "name": "getNodeInjectable" }, + { + "name": "getNonViewFirstChild" + }, { "name": "getOrCreateInjectable" }, diff --git a/packages/core/test/render3/di_spec.ts b/packages/core/test/render3/di_spec.ts index be5e453158..b80737ce5f 100644 --- a/packages/core/test/render3/di_spec.ts +++ b/packages/core/test/render3/di_spec.ts @@ -228,7 +228,7 @@ describe('di', () => { const contentView = createLView( null, createTView(TViewType.Component, -1, null, 1, 0, null, null, null, null, null), {}, LViewFlags.CheckAlways, null, null, {} as any, {} as any); - enterView(contentView, null); + enterView(contentView); try { const parentTNode = getOrCreateTNode(contentView[TVIEW], null, 0, TNodeType.Element, null, null); diff --git a/packages/core/test/render3/i18n_spec.ts b/packages/core/test/render3/i18n_spec.ts index 238a3a0d49..324f5fe41f 100644 --- a/packages/core/test/render3/i18n_spec.ts +++ b/packages/core/test/render3/i18n_spec.ts @@ -459,7 +459,9 @@ describe('Runtime i18n', () => { const nbConsts = 2; const index = 1; const opCodes = getOpCodes(attrs, () => { + ɵɵelementStart(0, 'div'); ɵɵi18nAttributes(index, 0); + ɵɵelementEnd(); }, null, nbConsts, index); expect(opCodes).toEqual(debugMatch([ @@ -473,7 +475,9 @@ describe('Runtime i18n', () => { const nbConsts = 2; const index = 1; const opCodes = getOpCodes(attrs, () => { + ɵɵelementStart(0, 'div'); ɵɵi18nAttributes(index, 0); + ɵɵelementEnd(); }, null, nbConsts, index); expect(opCodes).toEqual(debugMatch([ @@ -487,7 +491,9 @@ describe('Runtime i18n', () => { const nbConsts = 2; const index = 1; const opCodes = getOpCodes(attrs, () => { + ɵɵelementStart(0, 'div'); ɵɵi18nAttributes(index, 0); + ɵɵelementEnd(); }, null, nbConsts, index); expect(opCodes).toEqual(debugMatch([ diff --git a/packages/core/test/render3/instructions/lview_debug_spec.ts b/packages/core/test/render3/instructions/lview_debug_spec.ts index b0b69c9574..785c631c78 100644 --- a/packages/core/test/render3/instructions/lview_debug_spec.ts +++ b/packages/core/test/render3/instructions/lview_debug_spec.ts @@ -17,7 +17,7 @@ import {KeyValueArray} from '@angular/core/src/util/array_utils'; describe('lView_debug', () => { const mockFirstUpdatePassLView: LView = [null, {firstUpdatePass: true}] as any; - beforeEach(() => enterView(mockFirstUpdatePassLView, null)); + beforeEach(() => enterView(mockFirstUpdatePassLView)); afterEach(() => leaveView()); describe('TNode', () => { diff --git a/packages/core/test/render3/instructions/shared_spec.ts b/packages/core/test/render3/instructions/shared_spec.ts index 8d77e50548..5b3c2af38c 100644 --- a/packages/core/test/render3/instructions/shared_spec.ts +++ b/packages/core/test/render3/instructions/shared_spec.ts @@ -45,7 +45,7 @@ export function enterViewWithOneDiv() { null); lView[0 + HEADER_OFFSET] = div; tView.data[0 + HEADER_OFFSET] = tNode; - enterView(lView, tNode); + enterView(lView); } export function clearFirstUpdatePass() { diff --git a/packages/core/test/render3/render_util.ts b/packages/core/test/render3/render_util.ts index 5fb27646c0..803e2bb8ae 100644 --- a/packages/core/test/render3/render_util.ts +++ b/packages/core/test/render3/render_util.ts @@ -265,7 +265,7 @@ export function renderTemplate( const hostLView = createLView( null, tView, {}, LViewFlags.CheckAlways | LViewFlags.IsRoot, null, null, providedRendererFactory, renderer); - enterView(hostLView, null); + enterView(hostLView); const def: ComponentDef = ɵɵdefineComponent({ type: Object, diff --git a/packages/core/test/render3/styling_next/static_styling_spec.ts b/packages/core/test/render3/styling_next/static_styling_spec.ts index ddc0ea786c..1596e96db6 100644 --- a/packages/core/test/render3/styling_next/static_styling_spec.ts +++ b/packages/core/test/render3/styling_next/static_styling_spec.ts @@ -16,7 +16,7 @@ describe('static styling', () => { const mockFirstCreatePassLView: LView = [null, {firstCreatePass: true}] as any; let tNode!: TNode; beforeEach(() => { - enterView(mockFirstCreatePassLView, null); + enterView(mockFirstCreatePassLView); tNode = createTNode(null!, null!, TNodeType.Element, 0, '', null); }); it('should initialize when no attrs', () => { diff --git a/packages/core/test/render3/styling_next/style_binding_list_spec.ts b/packages/core/test/render3/styling_next/style_binding_list_spec.ts index b3f3c87033..6454bd432c 100644 --- a/packages/core/test/render3/styling_next/style_binding_list_spec.ts +++ b/packages/core/test/render3/styling_next/style_binding_list_spec.ts @@ -16,7 +16,7 @@ import {newArray} from '@angular/core/src/util/array_utils'; describe('TNode styling linked list', () => { const mockFirstUpdatePassLView: LView = [null, {firstUpdatePass: true}] as any; - beforeEach(() => enterView(mockFirstUpdatePassLView, null)); + beforeEach(() => enterView(mockFirstUpdatePassLView)); afterEach(() => leaveView()); describe('insertTStylingBinding', () => { it('should append template only', () => { diff --git a/packages/core/test/sanitization/sanitization_spec.ts b/packages/core/test/sanitization/sanitization_spec.ts index 7e7dfc07e0..389e28b52a 100644 --- a/packages/core/test/sanitization/sanitization_spec.ts +++ b/packages/core/test/sanitization/sanitization_spec.ts @@ -20,7 +20,7 @@ function fakeLView(): LView { } describe('sanitization', () => { - beforeEach(() => enterView(fakeLView(), null)); + beforeEach(() => enterView(fakeLView())); afterEach(() => leaveView()); class Wrap { constructor(private value: string) {}