From 2befc65777a444849a12b35cf7f70c9dc343051d Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Mon, 6 Aug 2018 14:49:35 +0200 Subject: [PATCH] fix(ivy): ngtsc should pay attention to declaration order (#25392) When generating the 'directives:' property of ngComponentDef, ngtsc needs to be conscious of declaration order. If a directive being written into the array is declarated after the component currently being compiled, then the entire directives array needs to be wrapped in a closure. This commit fixes ngtsc to pay attention to such ordering issues within directives arrays. PR Close #25392 --- .../src/ngtsc/annotations/src/component.ts | 5 +- .../ngtsc/annotations/src/selector_scope.ts | 47 ++++++++++++++++--- .../compliance/r3_compiler_compliance_spec.ts | 8 ++-- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 32 +++++++++++++ packages/compiler/src/render3/view/api.ts | 7 +++ .../compiler/src/render3/view/compiler.ts | 7 ++- packages/core/src/render3/jit/directive.ts | 1 + 7 files changed, 94 insertions(+), 13 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 9219755110..15b1967595 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -142,6 +142,7 @@ export class ComponentDecoratorHandler implements DecoratorHandler { directives: Map; pipes: Map; + containsForwardDecls?: boolean; } /** @@ -146,7 +147,7 @@ export class SelectorScopeRegistry { // The scope as cached is in terms of References, not Expressions. Converting between them // requires knowledge of the context file (in this case, the component node's source file). - return convertScopeToExpressions(scope, node.getSourceFile()); + return convertScopeToExpressions(scope, node); } // This is the first time the scope for this module is being computed. @@ -179,7 +180,7 @@ export class SelectorScopeRegistry { this._compilationScopeCache.set(node, scope); // Convert References to Expressions in the context of the component's source file. - return convertScopeToExpressions(scope, node.getSourceFile()); + return convertScopeToExpressions(scope, node); } /** @@ -390,8 +391,40 @@ function convertReferenceMap( } function convertScopeToExpressions( - scope: CompilationScope, context: ts.SourceFile): CompilationScope { - const directives = convertReferenceMap(scope.directives, context); - const pipes = convertReferenceMap(scope.pipes, context); - return {directives, pipes}; + scope: CompilationScope, context: ts.Declaration): CompilationScope { + const sourceContext = ts.getOriginalNode(context).getSourceFile(); + const directives = convertReferenceMap(scope.directives, sourceContext); + const pipes = convertReferenceMap(scope.pipes, sourceContext); + const declPointer = maybeUnwrapNameOfDeclaration(context); + let containsForwardDecls = false; + directives.forEach(expr => { + containsForwardDecls = + containsForwardDecls || isExpressionForwardReference(expr, declPointer, sourceContext); + }); + !containsForwardDecls && pipes.forEach(expr => { + containsForwardDecls = + containsForwardDecls || isExpressionForwardReference(expr, declPointer, sourceContext); + }); + return {directives, pipes, containsForwardDecls}; } + +function isExpressionForwardReference( + expr: Expression, context: ts.Node, contextSource: ts.SourceFile): boolean { + if (isWrappedTsNodeExpr(expr)) { + const node = ts.getOriginalNode(expr.node); + return node.getSourceFile() === contextSource && context.pos < node.pos; + } + return false; +} + +function isWrappedTsNodeExpr(expr: Expression): expr is WrappedNodeExpr { + return expr instanceof WrappedNodeExpr; +} + +function maybeUnwrapNameOfDeclaration(decl: ts.Declaration): ts.Declaration|ts.Identifier { + if ((ts.isClassDeclaration(decl) || ts.isVariableDeclaration(decl)) && decl.name !== undefined && + ts.isIdentifier(decl.name)) { + return decl.name; + } + return decl; +} \ No newline at end of file diff --git a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts index 15f5986888..0550a4c64b 100644 --- a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts @@ -1002,7 +1002,7 @@ describe('compiler compliance', () => { $r3$.ɵEe(1, "div", $e0_attrs$); } }, - directives:[SomeDirective] + directives: function () { return [SomeDirective]; } });`; const result = compile(files, angularFiles); @@ -1582,7 +1582,7 @@ describe('compiler compliance', () => { } if (rf & 2) { $r3$.ɵp(1,"forOf",$r3$.ɵb(ctx.items)); } }, - directives: [ForOfDirective] + directives: function() { return [ForOfDirective]; } }); `; @@ -1660,7 +1660,7 @@ describe('compiler compliance', () => { $r3$.ɵp(1, "forOf", $r3$.ɵb(ctx.items)); } }, - directives: [ForOfDirective] + directives: function() { return [ForOfDirective]; } }); `; @@ -1758,7 +1758,7 @@ describe('compiler compliance', () => { $r3$.ɵp(1, "forOf", $r3$.ɵb(ctx.items)); } }, - directives: [ForOfDirective] + directives: function () { return [ForOfDirective]; } });`; const result = compile(files, angularFiles); diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 4d7a45ee1d..1ea954a64b 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -715,4 +715,36 @@ describe('ngtsc behavioral tests', () => { expect(jsContents) .toContain('function GrandChild_Factory(t) { return new (t || GrandChild)(); }'); }); + + it('should wrap "directives" in component metadata in a closure when forward references are present', + () => { + writeConfig(); + write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'cmp-a', + template: '', + }) + class CmpA {} + + @Component({ + selector: 'cmp-b', + template: 'This is B', + }) + class CmpB {} + + @NgModule({ + declarations: [CmpA, CmpB], + }) + class Module {} + `); + + const exitCode = main(['-p', basePath], errorSpy); + expect(errorSpy).not.toHaveBeenCalled(); + expect(exitCode).toBe(0); + + const jsContents = getContents('test.js'); + expect(jsContents).toContain('directives: function () { return [CmpB]; }'); + }); }); diff --git a/packages/compiler/src/render3/view/api.ts b/packages/compiler/src/render3/view/api.ts index 6ee53ee6d2..aca42ed37f 100644 --- a/packages/compiler/src/render3/view/api.ts +++ b/packages/compiler/src/render3/view/api.ts @@ -144,6 +144,13 @@ export interface R3ComponentMetadata extends R3DirectiveMetadata { * scope of the compilation. */ directives: Map; + + /** + * Whether to wrap the 'directives' array, if one is generated, in a closure. + * + * This is done when the directives contain forward references. + */ + wrapDirectivesInClosure: boolean; } /** diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index 611931dcdc..b8a1c6f9e1 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -165,7 +165,11 @@ export function compileComponentFromMetadata( // e.g. `directives: [MyDirective]` if (directivesUsed.size) { - definitionMap.set('directives', o.literalArr(Array.from(directivesUsed))); + let directivesExpr: o.Expression = o.literalArr(Array.from(directivesUsed)); + if (meta.wrapDirectivesInClosure) { + directivesExpr = o.fn([], [new o.ReturnStatement(directivesExpr)]); + } + definitionMap.set('directives', directivesExpr); } // e.g. `pipes: [MyPipe]` @@ -241,6 +245,7 @@ export function compileComponentFromRender2( directives: typeMapToExpressionMap(directiveTypeBySel, outputCtx), pipes: typeMapToExpressionMap(pipeTypeByName, outputCtx), viewQueries: queriesFromGlobalMetadata(component.viewQueries, outputCtx), + wrapDirectivesInClosure: false, }; const res = compileComponentFromMetadata(meta, outputCtx.constantPool, bindingParser); diff --git a/packages/core/src/render3/jit/directive.ts b/packages/core/src/render3/jit/directive.ts index 2bc6cef0bb..210784fb11 100644 --- a/packages/core/src/render3/jit/directive.ts +++ b/packages/core/src/render3/jit/directive.ts @@ -73,6 +73,7 @@ export function compileComponent(type: Type, metadata: Component): void { directives: new Map(), pipes: new Map(), viewQueries: [], + wrapDirectivesInClosure: false, }, constantPool, makeBindingParser()); const preStatements = [...constantPool.statements, ...res.statements];