From 1e8b132ade6260e229d0b4031fea8dfcd93cf04a Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Tue, 21 Mar 2017 08:41:19 -0700 Subject: [PATCH] refactor(compiler): only produce `log` expressions for elements / text (#15350) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change reduces the amount of generated code by only adding `log` calls for elements and text nodes. We need the `log` calls to allow users to jump to the right place in the template via source maps. However, we only need it for element and text nodes, but not for directives, queries, … as for them we first locate the corresponding element or text node. Related to #15239 PR Close #15350 --- .../src/view_compiler/view_compiler.ts | 52 ++++++++++++------- packages/core/src/view/services.ts | 30 ++++++++--- .../source_map_integration_node_only_spec.ts | 33 +++++++++++- packages/core/test/view/services_spec.ts | 1 - 4 files changed, 87 insertions(+), 29 deletions(-) diff --git a/packages/compiler/src/view_compiler/view_compiler.ts b/packages/compiler/src/view_compiler/view_compiler.ts index 2651eee983..2d09942982 100644 --- a/packages/compiler/src/view_compiler/view_compiler.ts +++ b/packages/compiler/src/view_compiler/view_compiler.ts @@ -95,22 +95,19 @@ interface UpdateExpression { value: AST; } -const LOG_VAR = o.variable('log'); -const VIEW_VAR = o.variable('view'); -const CHECK_VAR = o.variable('check'); -const COMP_VAR = o.variable('comp'); -const NODE_INDEX_VAR = o.variable('nodeIndex'); -const EVENT_NAME_VAR = o.variable('eventName'); -const ALLOW_DEFAULT_VAR = o.variable(`allowDefault`); +const LOG_VAR = o.variable('l'); +const VIEW_VAR = o.variable('v'); +const CHECK_VAR = o.variable('ck'); +const COMP_VAR = o.variable('co'); +const EVENT_NAME_VAR = o.variable('en'); +const ALLOW_DEFAULT_VAR = o.variable(`ad`); class ViewBuilder implements TemplateAstVisitor, LocalResolver { private compType: o.Type; private nodes: (() => { sourceSpan: ParseSourceSpan, nodeDef: o.Expression, - directive?: CompileTypeMetadata, - updateDirectives?: UpdateExpression[], - updateRenderer?: UpdateExpression[] + nodeFlags: NodeFlags, updateDirectives?: UpdateExpression[], updateRenderer?: UpdateExpression[] })[] = []; private purePipeNodeIndices: {[pipeName: string]: number} = Object.create(null); // Need Object.create so that we don't have builtin values... @@ -158,6 +155,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { } this.nodes.push(() => ({ sourceSpan: null, + nodeFlags: flags, nodeDef: o.importExpr(createIdentifier(Identifiers.queryDef)).callFn([ o.literal(flags), o.literal(queryId), new o.LiteralMapExpr( @@ -171,6 +169,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // if the view is an embedded view, then we need to add an additional root node in some cases this.nodes.push(() => ({ sourceSpan: null, + nodeFlags: NodeFlags.TypeElement, nodeDef: o.importExpr(createIdentifier(Identifiers.anchorDef)).callFn([ o.literal(NodeFlags.None), o.NULL_EXPR, o.NULL_EXPR, o.literal(0) ]) @@ -229,6 +228,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // ngContentDef(ngContentIndex: number, index: number): NodeDef; this.nodes.push(() => ({ sourceSpan: ast.sourceSpan, + nodeFlags: NodeFlags.TypeNgContent, nodeDef: o.importExpr(createIdentifier(Identifiers.ngContentDef)).callFn([ o.literal(ast.ngContentIndex), o.literal(ast.index) ]) @@ -239,6 +239,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // textDef(ngContentIndex: number, constants: string[]): NodeDef; this.nodes.push(() => ({ sourceSpan: ast.sourceSpan, + nodeFlags: NodeFlags.TypeText, nodeDef: o.importExpr(createIdentifier(Identifiers.textDef)).callFn([ o.literal(ast.ngContentIndex), o.literalArr([o.literal(ast.value)]) ]) @@ -260,6 +261,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // textDef(ngContentIndex: number, constants: string[]): NodeDef; this.nodes[nodeIndex] = () => ({ sourceSpan: ast.sourceSpan, + nodeFlags: NodeFlags.TypeText, nodeDef: o.importExpr(createIdentifier(Identifiers.textDef)).callFn([ o.literal(ast.ngContentIndex), o.literalArr(inter.strings.map(s => o.literal(s))) ]), @@ -286,6 +288,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // ViewDefinitionFactory): NodeDef; this.nodes[nodeIndex] = () => ({ sourceSpan: ast.sourceSpan, + nodeFlags: NodeFlags.TypeElement | flags, nodeDef: o.importExpr(createIdentifier(Identifiers.anchorDef)).callFn([ o.literal(flags), queryMatchesExpr, @@ -360,6 +363,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // componentView?: () => ViewDefinition, componentRendererType?: RendererType2): NodeDef; this.nodes[nodeIndex] = () => ({ sourceSpan: ast.sourceSpan, + nodeFlags: NodeFlags.TypeElement | flags, nodeDef: o.importExpr(createIdentifier(Identifiers.elementDef)).callFn([ o.literal(flags), queryMatchesExpr, @@ -499,6 +503,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { const bindingType = query.first ? QueryBindingType.First : QueryBindingType.All; this.nodes.push(() => ({ sourceSpan: dirAst.sourceSpan, + nodeFlags: flags, nodeDef: o.importExpr(createIdentifier(Identifiers.queryDef)).callFn([ o.literal(flags), o.literal(queryId), new o.LiteralMapExpr( @@ -576,6 +581,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // outputs?: {[name: string]: string}, component?: () => ViewDefinition): NodeDef; this.nodes[nodeIndex] = () => ({ sourceSpan: dirAst.sourceSpan, + nodeFlags: NodeFlags.TypeDirective | flags, nodeDef: o.importExpr(createIdentifier(Identifiers.directiveDef)).callFn([ o.literal(flags), queryMatchExprs.length ? o.literalArr(queryMatchExprs) : o.NULL_EXPR, o.literal(childCount), providerExpr, depsExpr, @@ -602,6 +608,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // value: any, deps: ([DepFlags, any] | any)[]): NodeDef; this.nodes[nodeIndex] = () => ({ sourceSpan: providerAst.sourceSpan, + nodeFlags: flags, nodeDef: o.importExpr(createIdentifier(Identifiers.providerDef)).callFn([ o.literal(flags), queryMatchExprs.length ? o.literalArr(queryMatchExprs) : o.NULL_EXPR, tokenExpr(providerAst.token), providerExpr, depsExpr @@ -678,6 +685,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { this.nodes.push( () => ({ sourceSpan, + nodeFlags: NodeFlags.TypePureArray, nodeDef: o.importExpr(createIdentifier(Identifiers.pureArrayDef)).callFn([o.literal(argCount)]) })); @@ -694,6 +702,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // function pureObjectDef(propertyNames: string[]): NodeDef this.nodes.push(() => ({ sourceSpan, + nodeFlags: NodeFlags.TypePureObject, nodeDef: o.importExpr(createIdentifier(Identifiers.pureObjectDef)) .callFn([o.literalArr(keys.map(key => o.literal(key)))]) })); @@ -708,6 +717,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // function purePipeDef(argCount: number): NodeDef; this.nodes.push(() => ({ sourceSpan: expression.sourceSpan, + nodeFlags: NodeFlags.TypePurePipe, nodeDef: o.importExpr(createIdentifier(Identifiers.purePipeDef)) .callFn([o.literal(argCount)]) })); @@ -755,6 +765,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { // flags: NodeFlags, ctor: any, deps: ([DepFlags, any] | any)[]): NodeDef this.nodes.push(() => ({ sourceSpan, + nodeFlags: NodeFlags.TypePipe, nodeDef: o.importExpr(createIdentifier(Identifiers.pipeDef)).callFn([ o.literal(flags), o.importExpr(pipe.type), o.literalArr(depExprs) ]) @@ -792,26 +803,31 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { const updateRendererStmts: o.Statement[] = []; const updateDirectivesStmts: o.Statement[] = []; const nodeDefExprs = this.nodes.map((factory, nodeIndex) => { - const {nodeDef, directive, updateDirectives, updateRenderer, sourceSpan} = factory(); + const {nodeDef, nodeFlags, updateDirectives, updateRenderer, sourceSpan} = factory(); if (updateRenderer) { updateRendererStmts.push( - ...createUpdateStatements(nodeIndex, sourceSpan, updateRenderer, null)); + ...createUpdateStatements(nodeIndex, sourceSpan, updateRenderer, false)); } if (updateDirectives) { - updateDirectivesStmts.push( - ...createUpdateStatements(nodeIndex, sourceSpan, updateDirectives, directive)); + updateDirectivesStmts.push(...createUpdateStatements( + nodeIndex, sourceSpan, updateDirectives, + (nodeFlags & (NodeFlags.DoCheck | NodeFlags.OnInit)) > 0)); } // We use a comma expression to call the log function before // the nodeDef function, but still use the result of the nodeDef function // as the value. - const logWithNodeDef = new o.CommaExpr([LOG_VAR.callFn([]).callFn([]), nodeDef]); + // Note: We only add the logger to elements / text nodes, + // so we don't generate too much code. + const logWithNodeDef = nodeFlags & NodeFlags.CatRenderNode ? + new o.CommaExpr([LOG_VAR.callFn([]).callFn([]), nodeDef]) : + nodeDef; return o.applySourceSpanToExpressionIfNeeded(logWithNodeDef, sourceSpan); }); return {updateRendererStmts, updateDirectivesStmts, nodeDefExprs}; function createUpdateStatements( nodeIndex: number, sourceSpan: ParseSourceSpan, expressions: UpdateExpression[], - directive: CompileTypeMetadata): o.Statement[] { + allowEmptyExprs: boolean): o.Statement[] { const updateStmts: o.Statement[] = []; const exprs = expressions.map(({sourceSpan, context, value}) => { const bindingId = `${updateBindingCount++}`; @@ -822,9 +838,7 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { ...stmts.map(stmt => o.applySourceSpanToStatementIfNeeded(stmt, sourceSpan))); return o.applySourceSpanToExpressionIfNeeded(currValExpr, sourceSpan); }); - if (expressions.length || - (directive && (directive.lifecycleHooks.indexOf(LifecycleHooks.DoCheck) !== -1 || - directive.lifecycleHooks.indexOf(LifecycleHooks.OnInit) !== -1))) { + if (expressions.length || allowEmptyExprs) { updateStmts.push(o.applySourceSpanToStatementIfNeeded( callCheckStmt(nodeIndex, exprs).toStmt(), sourceSpan)); } diff --git a/packages/core/src/view/services.ts b/packages/core/src/view/services.ts index 39945d5eec..756c17d460 100644 --- a/packages/core/src/view/services.ts +++ b/packages/core/src/view/services.ts @@ -368,32 +368,46 @@ class DebugContext_ implements DebugContext { renderNode(this.elView, this.elDef); } logError(console: Console, ...values: any[]) { - let logViewFactory: ViewDefinitionFactory; + let logViewDef: ViewDefinition; let logNodeIndex: number; if (this.nodeDef.flags & NodeFlags.TypeText) { - logViewFactory = this.view.def.factory; + logViewDef = this.view.def; logNodeIndex = this.nodeDef.index; } else { - logViewFactory = this.elView.def.factory; + logViewDef = this.elView.def; logNodeIndex = this.elDef.index; } - let currNodeIndex = -1; + // Note: we only generate a log function for text and element nodes + // to make the generated code as small as possible. + const renderNodeIndex = getRenderNodeIndex(logViewDef, logNodeIndex); + let currRenderNodeIndex = -1; let nodeLogger: NodeLogger = () => { - currNodeIndex++; - if (currNodeIndex === logNodeIndex) { + currRenderNodeIndex++; + if (currRenderNodeIndex === renderNodeIndex) { return console.error.bind(console, ...values); } else { return NOOP; } }; - logViewFactory(nodeLogger); - if (currNodeIndex < logNodeIndex) { + logViewDef.factory(nodeLogger); + if (currRenderNodeIndex < renderNodeIndex) { console.error('Illegal state: the ViewDefinitionFactory did not call the logger!'); (console.error)(...values); } } } +function getRenderNodeIndex(viewDef: ViewDefinition, nodeIndex: number): number { + let renderNodeIndex = -1; + for (let i = 0; i <= nodeIndex; i++) { + const nodeDef = viewDef.nodes[i]; + if (nodeDef.flags & NodeFlags.CatRenderNode) { + renderNodeIndex++; + } + } + return renderNodeIndex; +} + function findHostElement(view: ViewData): ElementData { while (view && !isComponentView(view)) { view = view.parent; diff --git a/packages/core/test/linker/source_map_integration_node_only_spec.ts b/packages/core/test/linker/source_map_integration_node_only_spec.ts index 9c3cb368cb..11c7a97fc3 100644 --- a/packages/core/test/linker/source_map_integration_node_only_spec.ts +++ b/packages/core/test/linker/source_map_integration_node_only_spec.ts @@ -10,7 +10,7 @@ import {ResourceLoader} from '@angular/compiler'; import {SourceMap} from '@angular/compiler/src/output/source_map'; import {extractSourceMap, originalPositionFor} from '@angular/compiler/test/output/source_map_util'; import {MockResourceLoader} from '@angular/compiler/testing/src/resource_loader_mock'; -import {Component, Directive, ɵglobal} from '@angular/core'; +import {Attribute, Component, Directive, ɵglobal} from '@angular/core'; import {getErrorLogger} from '@angular/core/src/errors'; import {ComponentFixture, TestBed, fakeAsync, tick} from '@angular/core/testing'; @@ -159,6 +159,37 @@ export function main() { }); })); + it('should report di errors with multiple elements and directives', fakeAsync(() => { + const template = `
`; + + @Component({...templateDecorator(template)}) + class MyComp { + } + + @Directive({selector: '[someDir]'}) + class SomeDir { + constructor(@Attribute('someDir') someDir: string) { + if (someDir === 'throw') { + throw new Error('Test'); + } + } + } + + TestBed.configureTestingModule({declarations: [SomeDir]}); + let error: any; + try { + compileAndCreateComponent(MyComp); + } catch (e) { + error = e; + } + // The error should be logged from the 2nd-element + expect(getSourcePositionForStack(getErrorLoggerStack(error))).toEqual({ + line: 1, + column: 19, + source: ngUrl, + }); + })); + it('should report source location for binding errors', fakeAsync(() => { const template = `
\n
`; diff --git a/packages/core/test/view/services_spec.ts b/packages/core/test/view/services_spec.ts index 66c813a135..5f20d4715c 100644 --- a/packages/core/test/view/services_spec.ts +++ b/packages/core/test/view/services_spec.ts @@ -83,7 +83,6 @@ export function main() { expect(debugCtx.renderNode).toBe(asElementData(compView, 0).renderElement); }); - }); }); }