diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index f4310af438..53f00a6fdb 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ConstantPool, CssSelector, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, Expression, ExternalExpr, InterpolationConfig, LexerRange, ParseError, R3ComponentMetadata, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr, compileComponentFromMetadata, makeBindingParser, parseTemplate} from '@angular/compiler'; +import {BoundTarget, ConstantPool, CssSelector, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, Expression, ExternalExpr, InterpolationConfig, LexerRange, ParseError, R3ComponentMetadata, R3TargetBinder, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr, compileComponentFromMetadata, makeBindingParser, parseTemplate} from '@angular/compiler'; import * as path from 'path'; import * as ts from 'typescript'; @@ -48,6 +48,7 @@ export class ComponentDecoratorHandler implements private refEmitter: ReferenceEmitter) {} private literalCache = new Map(); + private boundTemplateCache = new Map>(); private elementSchemaRegistry = new DomElementSchemaRegistry(); /** @@ -323,7 +324,8 @@ export class ComponentDecoratorHandler implements for (const meta of scope.compilation.directives) { matcher.addSelectables(CssSelector.parse(meta.selector), meta); } - ctx.addTemplate(node as ts.ClassDeclaration, meta.parsedTemplate, matcher); + const bound = new R3TargetBinder(matcher).bind({template: meta.parsedTemplate}); + ctx.addTemplate(node, bound); } } @@ -337,8 +339,17 @@ export class ComponentDecoratorHandler implements // Replace the empty components and directives from the analyze() step with a fully expanded // scope. This is possible now because during resolve() the whole compilation unit has been // fully analyzed. - const directives = scope.compilation.directives.map( - dir => ({selector: dir.selector, expression: this.refEmitter.emit(dir.ref, context)})); + + const matcher = new SelectorMatcher(); + const directives: {selector: string, expression: Expression}[] = []; + + for (const dir of scope.compilation.directives) { + const {ref, selector} = dir; + const expression = this.refEmitter.emit(ref, context); + directives.push({selector, expression}); + + matcher.addSelectables(CssSelector.parse(selector), {...dir, expression}); + } const pipes = new Map(); for (const pipe of scope.compilation.pipes) { pipes.set(pipe.name, this.refEmitter.emit(pipe.ref, context)); @@ -346,18 +357,35 @@ export class ComponentDecoratorHandler implements // Scan through the references of the `scope.directives` array and check whether // any import which needs to be generated for the directive would create a cycle. - const origin = node.getSourceFile(); - const cycleDetected = directives.some(dir => this._isCyclicImport(dir.expression, origin)) || - Array.from(pipes.values()).some(pipe => this._isCyclicImport(pipe, origin)); + const cycleDetected = directives.some(dir => this._isCyclicImport(dir.expression, context)) || + Array.from(pipes.values()).some(pipe => this._isCyclicImport(pipe, context)); if (!cycleDetected) { const wrapDirectivesAndPipesInClosure = directives.some( - dir => isExpressionForwardReference(dir.expression, node.name !, origin)) || + dir => isExpressionForwardReference(dir.expression, node.name !, context)) || Array.from(pipes.values()) - .some(pipe => isExpressionForwardReference(pipe, node.name !, origin)); + .some(pipe => isExpressionForwardReference(pipe, node.name !, context)); metadata.directives = directives; metadata.pipes = pipes; metadata.wrapDirectivesAndPipesInClosure = wrapDirectivesAndPipesInClosure; + for (const dir of directives) { + this._recordSyntheticImport(dir.expression, context); + } + for (const [name, pipe] of pipes) { + this._recordSyntheticImport(pipe, context); + } + + const binder = new R3TargetBinder(matcher); + const bound = binder.bind({template: metadata.template.nodes}); + for (const {expression} of bound.getUsedDirectives()) { + this._recordSyntheticImport(expression, context); + } + + for (const name of bound.getUsedPipes()) { + if (pipes.has(name)) { + this._recordSyntheticImport(pipes.get(name) !, context); + } + } } else { this.scopeRegistry.setComponentAsRequiringRemoteScoping(node); } @@ -549,13 +577,17 @@ export class ComponentDecoratorHandler implements }; } - private _isCyclicImport(expr: Expression, origin: ts.SourceFile): boolean { + private _expressionToImportedFile(expr: Expression, origin: ts.SourceFile): ts.SourceFile|null { if (!(expr instanceof ExternalExpr)) { - return false; + return null; } // Figure out what file is being imported. - const imported = this.moduleResolver.resolveModuleName(expr.value.moduleName !, origin); + return this.moduleResolver.resolveModuleName(expr.value.moduleName !, origin); + } + + private _isCyclicImport(expr: Expression, origin: ts.SourceFile): boolean { + const imported = this._expressionToImportedFile(expr, origin); if (imported === null) { return false; } @@ -563,6 +595,15 @@ export class ComponentDecoratorHandler implements // Check whether the import is legal. return this.cycleAnalyzer.wouldCreateCycle(origin, imported); } + + private _recordSyntheticImport(expr: Expression, origin: ts.SourceFile): void { + const imported = this._expressionToImportedFile(expr, origin); + if (imported === null) { + return; + } + + this.cycleAnalyzer.recordSyntheticImport(origin, imported); + } } function getTemplateRange(templateExpr: ts.Expression) { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index 43500d5955..b9c7ab09d3 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {R3TargetBinder, SelectorMatcher, TmplAstNode} from '@angular/compiler'; +import {BoundTarget} from '@angular/compiler'; import * as ts from 'typescript'; import {NoopImportRewriter, ReferenceEmitter} from '../../imports'; @@ -47,22 +47,13 @@ export class TypeCheckContext { * @param template AST nodes of the template being recorded. * @param matcher `SelectorMatcher` which tracks directives that are in scope for this template. */ - addTemplate( - node: ts.ClassDeclaration, template: TmplAstNode[], - matcher: SelectorMatcher): void { + addTemplate(node: ts.ClassDeclaration, boundTarget: BoundTarget): + void { // Only write TCBs for named classes. if (node.name === undefined) { throw new Error(`Assertion: class must be named`); } - // Bind the template, which will: - // - Extract the metadata needed to generate type check blocks. - // - Perform directive matching, which informs the context which directives are used in the - // template. This allows generation of type constructors for only those directives which - // are actually used by the templates. - const binder = new R3TargetBinder(matcher); - const boundTarget = binder.bind({template}); - // Get all of the directives used in the template and record type constructors for all of them. boundTarget.getUsedDirectives().forEach(dir => { const dirNode = dir.ref.node; diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 9fcb0c8f1a..9c2c20c57e 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -2045,6 +2045,41 @@ describe('ngtsc behavioral tests', () => { /i\d\.ɵsetComponentScope\(NormalComponent,\s+\[NormalComponent,\s+CyclicComponent\],\s+\[\]\)/); expect(jsContents).not.toContain('/*__PURE__*/ i0.ɵsetComponentScope'); }); + + it('should detect a cycle added entirely during compilation', () => { + env.tsconfig(); + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + import {ACmp} from './a'; + import {BCmp} from './b'; + + @NgModule({declarations: [ACmp, BCmp]}) + export class Module {} + `); + env.write('a.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'a-cmp', + template: '', + }) + export class ACmp {} + `); + env.write('b.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'b-cmp', + template: '', + }) + export class BCmp {} + `); + env.driveMain(); + const aJsContents = env.getContents('a.js'); + const bJsContents = env.getContents('b.js'); + expect(aJsContents).toMatch(/import \* as i\d? from ".\/b"/); + expect(bJsContents).not.toMatch(/import \* as i\d? from ".\/a"/); + }); }); describe('multiple local refs', () => { diff --git a/packages/compiler/src/render3/view/t2_api.ts b/packages/compiler/src/render3/view/t2_api.ts index e484760e86..8b5990e52d 100644 --- a/packages/compiler/src/render3/view/t2_api.ts +++ b/packages/compiler/src/render3/view/t2_api.ts @@ -136,4 +136,9 @@ export interface BoundTarget { * Get a list of all the directives used by the target. */ getUsedDirectives(): DirectiveT[]; + + /** + * Get a list of all the pipes used by the target. + */ + getUsedPipes(): string[]; } diff --git a/packages/compiler/src/render3/view/t2_binder.ts b/packages/compiler/src/render3/view/t2_binder.ts index 2cde692372..fdab572717 100644 --- a/packages/compiler/src/render3/view/t2_binder.ts +++ b/packages/compiler/src/render3/view/t2_binder.ts @@ -6,13 +6,14 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, ImplicitReceiver, MethodCall, PropertyRead, PropertyWrite, RecursiveAstVisitor, SafeMethodCall, SafePropertyRead} from '../../expression_parser/ast'; +import {AST, BindingPipe, ImplicitReceiver, MethodCall, PropertyRead, PropertyWrite, RecursiveAstVisitor, SafeMethodCall, SafePropertyRead} from '../../expression_parser/ast'; import {CssSelector, SelectorMatcher} from '../../selector'; import {BoundAttribute, BoundEvent, BoundText, Content, Element, Icu, Node, Reference, Template, Text, TextAttribute, Variable, Visitor} from '../r3_ast'; import {BoundTarget, DirectiveMeta, Target, TargetBinder} from './t2_api'; import {getAttrsForDirectiveMatching} from './util'; + /** * Processes `Target`s with a given set of directives and performs a binding operation, which * returns an object similar to TypeScript's `ts.TypeChecker` that contains knowledge about the @@ -44,9 +45,10 @@ export class R3TargetBinder implements TargetB DirectiveBinder.apply(target.template, this.directiveMatcher); // Finally, run the TemplateBinder to bind references, variables, and other entities within the // template. This extracts all the metadata that doesn't depend on directive matching. - const {expressions, symbols, nestingLevel} = TemplateBinder.apply(target.template, scope); + const {expressions, symbols, nestingLevel, usedPipes} = + TemplateBinder.apply(target.template, scope); return new R3BoundTarget( - target, directives, bindings, references, expressions, symbols, nestingLevel); + target, directives, bindings, references, expressions, symbols, nestingLevel, usedPipes); } } @@ -331,9 +333,11 @@ class DirectiveBinder implements Visitor { class TemplateBinder extends RecursiveAstVisitor implements Visitor { private visitNode: (node: Node) => void; + private pipesUsed: string[] = []; + private constructor( private bindings: Map, - private symbols: Map, + private symbols: Map, private usedPipes: Set, private nestingLevel: Map, private scope: Scope, private template: Template|null, private level: number) { super(); @@ -358,16 +362,18 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor { expressions: Map, symbols: Map, nestingLevel: Map, + usedPipes: Set, } { const expressions = new Map(); const symbols = new Map(); const nestingLevel = new Map(); + const usedPipes = new Set(); // The top-level template has nesting level 0. const binder = new TemplateBinder( - expressions, symbols, nestingLevel, scope, template instanceof Template ? template : null, - 0); + expressions, symbols, usedPipes, nestingLevel, scope, + template instanceof Template ? template : null, 0); binder.ingest(template); - return {expressions, symbols, nestingLevel}; + return {expressions, symbols, nestingLevel, usedPipes}; } private ingest(template: Template|Node[]): void { @@ -405,7 +411,8 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor { // Next, recurse into the template using its scope, and bumping the nesting level up by one. const childScope = this.scope.getChildScope(template); const binder = new TemplateBinder( - this.bindings, this.symbols, this.nestingLevel, childScope, template, this.level + 1); + this.bindings, this.symbols, this.usedPipes, this.nestingLevel, childScope, template, + this.level + 1); binder.ingest(template); } @@ -437,8 +444,10 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor { visitBoundEvent(event: BoundEvent) { event.handler.visit(this); } visitBoundText(text: BoundText) { text.value.visit(this); } - - + visitPipe(ast: BindingPipe, context: any): any { + this.usedPipes.add(ast.name); + return super.visitPipe(ast, context); + } // These five types of AST expressions can refer to expression roots, which could be variables // or references in the current scope. @@ -500,7 +509,7 @@ export class R3BoundTarget implements BoundTar {directive: DirectiveT, node: Element|Template}|Element|Template>, private exprTargets: Map, private symbols: Map, - private nestingLevel: Map) {} + private nestingLevel: Map, private usedPipes: Set) {} getDirectivesOfNode(node: Element|Template): DirectiveT[]|null { return this.directives.get(node) || null; @@ -531,4 +540,6 @@ export class R3BoundTarget implements BoundTar this.directives.forEach(dirs => dirs.forEach(dir => set.add(dir))); return Array.from(set.values()); } + + getUsedPipes(): string[] { return Array.from(this.usedPipes); } }