From 7d954dffd0fee7f6e8048e55c2b07a7f695ddf7d Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 15 Jan 2019 12:32:10 -0800 Subject: [PATCH] feat(ivy): detect cycles and use remote scoping of components if needed (#28169) By its nature, Ivy alters the import graph of a TS program, adding imports where template dependencies exist. For example, if ComponentA uses PipeB in its template, Ivy will insert an import of PipeB into the file in which ComponentA is declared. Any insertion of an import into a program has the potential to introduce a cycle into the import graph. If for some reason the file in which PipeB is declared imports the file in which ComponentA is declared (maybe it makes use of a service or utility function that happens to be in the same file as ComponentA) then this could create an import cycle. This turns out to happen quite regularly in larger Angular codebases. TypeScript and the Ivy runtime have no issues with such cycles. However, other tools are not so accepting. In particular the Closure Compiler is very anti-cycle. To mitigate this problem, it's necessary to detect when the insertion of an import would create a cycle. ngtsc can then use a different strategy, known as "remote scoping", instead of directly writing a reference from one component to another. Under remote scoping, a function 'setComponentScope' is called after the declaration of the component's module, which does not require the addition of new imports. FW-647 #resolve PR Close #28169 --- packages/compiler-cli/BUILD.bazel | 1 + packages/compiler-cli/src/ngcc/BUILD.bazel | 1 + .../ngcc/src/analysis/decoration_analyzer.ts | 10 +- .../src/ngtsc/annotations/BUILD.bazel | 1 + .../src/ngtsc/annotations/src/component.ts | 54 +- .../src/ngtsc/annotations/src/ng_module.ts | 26 +- .../ngtsc/annotations/src/selector_scope.ts | 20 + .../src/ngtsc/annotations/test/BUILD.bazel | 1 + .../ngtsc/annotations/test/component_spec.ts | 9 +- packages/compiler-cli/src/ngtsc/program.ts | 7 +- .../src/ngtsc/transform/src/api.ts | 9 + .../src/ngtsc/transform/src/compilation.ts | 8 + .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 40 +- packages/compiler/src/compiler.ts | 1 + .../compiler/src/render3/r3_identifiers.ts | 2 + .../core/src/core_render3_private_export.ts | 1 + packages/core/src/render3/definition.ts | 7 + packages/core/src/render3/index.ts | 3 +- packages/core/src/render3/jit/environment.ts | 1 + .../test/bundling/cyclic_import/BUILD.bazel | 88 +++ .../test/bundling/cyclic_import/README.md | 22 + .../cyclic_import/bundle.golden_symbols.json | 644 ++++++++++++++++++ .../test/bundling/cyclic_import/index.html | 31 + .../core/test/bundling/cyclic_import/index.ts | 26 + .../cyclic_import/integration_spec.ts | 41 ++ .../test/bundling/cyclic_import/trigger.ts | 16 + packages/core/test/render3/ivy/jit_spec.ts | 2 +- 27 files changed, 1049 insertions(+), 23 deletions(-) create mode 100644 packages/core/test/bundling/cyclic_import/BUILD.bazel create mode 100644 packages/core/test/bundling/cyclic_import/README.md create mode 100644 packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json create mode 100644 packages/core/test/bundling/cyclic_import/index.html create mode 100644 packages/core/test/bundling/cyclic_import/index.ts create mode 100644 packages/core/test/bundling/cyclic_import/integration_spec.ts create mode 100644 packages/core/test/bundling/cyclic_import/trigger.ts diff --git a/packages/compiler-cli/BUILD.bazel b/packages/compiler-cli/BUILD.bazel index 78390cb36e..d7432a145d 100644 --- a/packages/compiler-cli/BUILD.bazel +++ b/packages/compiler-cli/BUILD.bazel @@ -24,6 +24,7 @@ ts_library( deps = [ "//packages/compiler", "//packages/compiler-cli/src/ngtsc/annotations", + "//packages/compiler-cli/src/ngtsc/cycles", "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/entry_point", "//packages/compiler-cli/src/ngtsc/imports", diff --git a/packages/compiler-cli/src/ngcc/BUILD.bazel b/packages/compiler-cli/src/ngcc/BUILD.bazel index 6606098c1e..bcc5f8f01f 100644 --- a/packages/compiler-cli/src/ngcc/BUILD.bazel +++ b/packages/compiler-cli/src/ngcc/BUILD.bazel @@ -12,6 +12,7 @@ ts_library( "//packages:types", "//packages/compiler", "//packages/compiler-cli/src/ngtsc/annotations", + "//packages/compiler-cli/src/ngtsc/cycles", "//packages/compiler-cli/src/ngtsc/imports", "//packages/compiler-cli/src/ngtsc/partial_evaluator", "//packages/compiler-cli/src/ngtsc/reflection", diff --git a/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts index 5745516138..84d747fc98 100644 --- a/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/src/ngcc/src/analysis/decoration_analyzer.ts @@ -11,7 +11,8 @@ import * as fs from 'fs'; import * as ts from 'typescript'; import {BaseDefDecoratorHandler, ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader, SelectorScopeRegistry} from '../../../ngtsc/annotations'; -import {TsReferenceResolver} from '../../../ngtsc/imports'; +import {CycleAnalyzer, ImportGraph} from '../../../ngtsc/cycles'; +import {ModuleResolver, TsReferenceResolver} from '../../../ngtsc/imports'; import {PartialEvaluator} from '../../../ngtsc/partial_evaluator'; import {CompileResult, DecoratorHandler} from '../../../ngtsc/transform'; import {DecoratedClass} from '../host/decorated_class'; @@ -65,12 +66,15 @@ export class DecorationAnalyzer { resolver = new TsReferenceResolver(this.program, this.typeChecker, this.options, this.host); scopeRegistry = new SelectorScopeRegistry(this.typeChecker, this.reflectionHost, this.resolver); evaluator = new PartialEvaluator(this.reflectionHost, this.typeChecker, this.resolver); + moduleResolver = new ModuleResolver(this.program, this.options, this.host); + importGraph = new ImportGraph(this.moduleResolver); + cycleAnalyzer = new CycleAnalyzer(this.importGraph); handlers: DecoratorHandler[] = [ new BaseDefDecoratorHandler(this.reflectionHost, this.evaluator), new ComponentDecoratorHandler( this.reflectionHost, this.evaluator, this.scopeRegistry, this.isCore, this.resourceManager, - this.rootDirs, /* defaultPreserveWhitespaces */ false, - /* i18nUseExternalIds */ true), + this.rootDirs, /* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true, + this.moduleResolver, this.cycleAnalyzer), new DirectiveDecoratorHandler( this.reflectionHost, this.evaluator, this.scopeRegistry, this.isCore), new InjectableDecoratorHandler(this.reflectionHost, this.isCore), diff --git a/packages/compiler-cli/src/ngtsc/annotations/BUILD.bazel b/packages/compiler-cli/src/ngtsc/annotations/BUILD.bazel index 8ccbfcec29..11653d1920 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/annotations/BUILD.bazel @@ -10,6 +10,7 @@ ts_library( ]), deps = [ "//packages/compiler", + "//packages/compiler-cli/src/ngtsc/cycles", "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/imports", "//packages/compiler-cli/src/ngtsc/partial_evaluator", diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 418da51c9b..93acc62257 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -6,12 +6,13 @@ * found in the LICENSE file at https://angular.io/license */ -import {ConstantPool, CssSelector, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, ElementSchemaRegistry, Expression, InterpolationConfig, R3ComponentMetadata, R3DirectiveMetadata, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr, compileComponentFromMetadata, makeBindingParser, parseTemplate} from '@angular/compiler'; +import {ConstantPool, CssSelector, DEFAULT_INTERPOLATION_CONFIG, DomElementSchemaRegistry, ElementSchemaRegistry, Expression, ExternalExpr, InterpolationConfig, R3ComponentMetadata, R3DirectiveMetadata, SelectorMatcher, Statement, TmplAstNode, WrappedNodeExpr, compileComponentFromMetadata, makeBindingParser, parseTemplate} from '@angular/compiler'; import * as path from 'path'; import * as ts from 'typescript'; +import {CycleAnalyzer} from '../../cycles'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; -import {ResolvedReference} from '../../imports'; +import {ModuleResolver, Reference, ResolvedReference} from '../../imports'; import {EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {Decorator, ReflectionHost, filterToMembersWithDecorator, reflectObjectLiteral} from '../../reflection'; import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; @@ -41,7 +42,8 @@ export class ComponentDecoratorHandler implements private reflector: ReflectionHost, private evaluator: PartialEvaluator, private scopeRegistry: SelectorScopeRegistry, private isCore: boolean, private resourceLoader: ResourceLoader, private rootDirs: string[], - private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean) {} + private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean, + private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer) {} private literalCache = new Map(); private elementSchemaRegistry = new DomElementSchemaRegistry(); @@ -289,28 +291,39 @@ export class ComponentDecoratorHandler implements } } - compile(node: ts.ClassDeclaration, analysis: ComponentHandlerData, pool: ConstantPool): - CompileResult { + resolve(node: ts.ClassDeclaration, analysis: ComponentHandlerData): void { // Check whether this component was registered with an NgModule. If so, it should be compiled // under that module's compilation scope. const scope = this.scopeRegistry.lookupCompilationScope(node); let metadata = analysis.meta; if (scope !== null) { // Replace the empty components and directives from the analyze() step with a fully expanded - // scope. This is possible now because during compile() the whole compilation unit has been + // scope. This is possible now because during resolve() the whole compilation unit has been // fully analyzed. const {pipes, containsForwardDecls} = scope; - const directives: {selector: string, expression: Expression}[] = []; + const directives = + scope.directives.map(dir => ({selector: dir.selector, expression: dir.directive})); - for (const meta of scope.directives) { - directives.push({selector: meta.selector, expression: meta.directive}); + // 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 = + scope.directives.some(meta => this._isCyclicImport(meta.directive, origin)) || + Array.from(scope.pipes.values()).some(pipe => this._isCyclicImport(pipe, origin)); + if (!cycleDetected) { + const wrapDirectivesAndPipesInClosure: boolean = !!containsForwardDecls; + metadata.directives = directives; + metadata.pipes = pipes; + metadata.wrapDirectivesAndPipesInClosure = wrapDirectivesAndPipesInClosure; + } else { + this.scopeRegistry.setComponentAsRequiringRemoteScoping(node); } - const wrapDirectivesAndPipesInClosure: boolean = !!containsForwardDecls; - metadata = {...metadata, directives, pipes, wrapDirectivesAndPipesInClosure}; } + } - const res = - compileComponentFromMetadata(metadata, pool, makeBindingParser(metadata.interpolation)); + compile(node: ts.ClassDeclaration, analysis: ComponentHandlerData, pool: ConstantPool): + CompileResult { + const res = compileComponentFromMetadata(analysis.meta, pool, makeBindingParser()); const statements = res.statements; if (analysis.metadataStmt !== null) { @@ -373,4 +386,19 @@ export class ComponentDecoratorHandler implements } return styleUrls as string[]; } + + private _isCyclicImport(expr: Expression, origin: ts.SourceFile): boolean { + if (!(expr instanceof ExternalExpr)) { + return false; + } + + // Figure out what file is being imported. + const imported = this.moduleResolver.resolveModuleName(expr.value.moduleName !, origin); + if (imported === null) { + return false; + } + + // Check whether the import is legal. + return this.cycleAnalyzer.wouldCreateCycle(origin, imported); + } } diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index b19df3c058..f8a93ae3f2 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Expression, LiteralArrayExpr, R3InjectorMetadata, R3NgModuleMetadata, R3Reference, Statement, WrappedNodeExpr, compileInjector, compileNgModule} from '@angular/compiler'; +import {Expression, ExternalExpr, InvokeFunctionExpr, LiteralArrayExpr, R3Identifiers, R3InjectorMetadata, R3NgModuleMetadata, R3Reference, Statement, WrappedNodeExpr, compileInjector, compileNgModule} from '@angular/compiler'; import * as ts from 'typescript'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; @@ -25,6 +25,7 @@ export interface NgModuleAnalysis { ngModuleDef: R3NgModuleMetadata; ngInjectorDef: R3InjectorMetadata; metadataStmt: Statement|null; + declarations: Reference[]; } /** @@ -152,6 +153,7 @@ export class NgModuleDecoratorHandler implements DecoratorHandler { directives.push(directive.ref.toExpression(context) !); }); + scope.pipes.forEach(pipe => pipes.push(pipe.toExpression(context) !)); + const directiveArray = new LiteralArrayExpr(directives); + const pipesArray = new LiteralArrayExpr(pipes); + const declExpr = decl.toExpression(context) !; + const setComponentScope = new ExternalExpr(R3Identifiers.setComponentScope); + const callExpr = + new InvokeFunctionExpr(setComponentScope, [declExpr, directiveArray, pipesArray]); + + ngModuleStatements.push(callExpr.toStmt()); + } + } return [ { name: 'ngModuleDef', diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts b/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts index 65a853ac8e..6fb342868a 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts @@ -84,6 +84,11 @@ export class SelectorScopeRegistry { */ private _pipeToName = new Map(); + /** + * Components that require remote scoping. + */ + private _requiresRemoteScope = new Set(); + /** * Map of components/directives/pipes to their module. */ @@ -132,6 +137,21 @@ export class SelectorScopeRegistry { this._pipeToName.set(node, name); } + /** + * Mark a component (identified by its `ts.Declaration`) as requiring its `directives` scope to be + * set remotely, from the file of the @NgModule which declares the component. + */ + setComponentAsRequiringRemoteScoping(component: ts.Declaration): void { + this._requiresRemoteScope.add(component); + } + + /** + * Check whether the given component requires its `directives` scope to be set remotely. + */ + requiresRemoteScope(component: ts.Declaration): boolean { + return this._requiresRemoteScope.has(ts.getOriginalNode(component) as ts.Declaration); + } + lookupCompilationScopeAsRefs(node: ts.Declaration): CompilationScope|null { node = ts.getOriginalNode(node) as ts.Declaration; diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/annotations/test/BUILD.bazel index 5b5ce5bd17..0379b2da14 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/annotations/test/BUILD.bazel @@ -12,6 +12,7 @@ ts_library( "//packages:types", "//packages/compiler", "//packages/compiler-cli/src/ngtsc/annotations", + "//packages/compiler-cli/src/ngtsc/cycles", "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/imports", "//packages/compiler-cli/src/ngtsc/partial_evaluator", diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts index 40ceb55e41..42256a0670 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts @@ -8,8 +8,9 @@ import * as ts from 'typescript'; +import {CycleAnalyzer, ImportGraph} from '../../cycles'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; -import {TsReferenceResolver} from '../../imports'; +import {ModuleResolver, TsReferenceResolver} from '../../imports'; import {PartialEvaluator} from '../../partial_evaluator'; import {TypeScriptReflectionHost} from '../../reflection'; import {getDeclaration, makeProgram} from '../../testing/in_memory_typescript'; @@ -45,9 +46,13 @@ describe('ComponentDecoratorHandler', () => { const reflectionHost = new TypeScriptReflectionHost(checker); const resolver = new TsReferenceResolver(program, checker, options, host); const evaluator = new PartialEvaluator(reflectionHost, checker, resolver); + const moduleResolver = new ModuleResolver(program, options, host); + const importGraph = new ImportGraph(moduleResolver); + const cycleAnalyzer = new CycleAnalyzer(importGraph); + const handler = new ComponentDecoratorHandler( reflectionHost, evaluator, new SelectorScopeRegistry(checker, reflectionHost, resolver), - false, new NoopResourceLoader(), [''], false, true); + false, new NoopResourceLoader(), [''], false, true, moduleResolver, cycleAnalyzer); const TestCmp = getDeclaration(program, 'entry.ts', 'TestCmp', ts.isClassDeclaration); const detected = handler.detect(TestCmp, reflectionHost.getDecoratorsOfDeclaration(TestCmp)); if (detected === undefined) { diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 9128135ad5..69a683aa59 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -14,6 +14,7 @@ import {nocollapseHack} from '../transformers/nocollapse_hack'; import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, NoopReferencesRegistry, PipeDecoratorHandler, ReferencesRegistry, SelectorScopeRegistry} from './annotations'; import {BaseDefDecoratorHandler} from './annotations/src/base_def'; +import {CycleAnalyzer, ImportGraph} from './cycles'; import {ErrorCode, ngErrorCode} from './diagnostics'; import {FlatIndexGenerator, ReferenceGraph, checkForPrivateExports, findFlatIndexEntryPoint} from './entry_point'; import {ImportRewriter, ModuleResolver, NoopImportRewriter, R3SymbolsImportRewriter, Reference, TsReferenceResolver} from './imports'; @@ -48,6 +49,7 @@ export class NgtscProgram implements api.Program { private constructionDiagnostics: ts.Diagnostic[] = []; private moduleResolver: ModuleResolver; + private cycleAnalyzer: CycleAnalyzer; constructor( @@ -128,6 +130,7 @@ export class NgtscProgram implements api.Program { this.entryPoint = entryPoint !== null ? this.tsProgram.getSourceFile(entryPoint) || null : null; this.moduleResolver = new ModuleResolver(this.tsProgram, options, this.host); + this.cycleAnalyzer = new CycleAnalyzer(new ImportGraph(this.moduleResolver)); } getTsProgram(): ts.Program { return this.tsProgram; } @@ -184,6 +187,7 @@ export class NgtscProgram implements api.Program { .filter(file => !file.fileName.endsWith('.d.ts')) .map(file => this.compilation !.analyzeAsync(file)) .filter((result): result is Promise => result !== undefined)); + this.compilation.resolve(); } listLazyRoutes(entryRoute?: string|undefined): api.LazyRoute[] { @@ -213,6 +217,7 @@ export class NgtscProgram implements api.Program { this.tsProgram.getSourceFiles() .filter(file => !file.fileName.endsWith('.d.ts')) .forEach(file => this.compilation !.analyzeSync(file)); + this.compilation.resolve(); } return this.compilation; } @@ -307,7 +312,7 @@ export class NgtscProgram implements api.Program { new ComponentDecoratorHandler( this.reflector, evaluator, scopeRegistry, this.isCore, this.resourceManager, this.rootDirs, this.options.preserveWhitespaces || false, - this.options.i18nUseExternalIds !== false), + this.options.i18nUseExternalIds !== false, this.moduleResolver, this.cycleAnalyzer), new DirectiveDecoratorHandler(this.reflector, evaluator, scopeRegistry, this.isCore), new InjectableDecoratorHandler(this.reflector, this.isCore), new NgModuleDecoratorHandler( diff --git a/packages/compiler-cli/src/ngtsc/transform/src/api.ts b/packages/compiler-cli/src/ngtsc/transform/src/api.ts index 6580e425ad..31f146c202 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/api.ts @@ -44,6 +44,15 @@ export interface DecoratorHandler { */ analyze(node: ts.Declaration, metadata: M): AnalysisOutput; + /** + * Perform resolution on the given decorator along with the result of analysis. + * + * The resolution phase happens after the entire `ts.Program` has been analyzed, and gives the + * `DecoratorHandler` a chance to leverage information from the whole compilation unit to enhance + * the `analysis` before the emit phase. + */ + resolve?(node: ts.Declaration, analysis: A): void; + typeCheck?(ctx: TypeCheckContext, node: ts.Declaration, metadata: A): void; /** diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index 3e65c93479..6e277d8125 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -163,6 +163,14 @@ export class IvyCompilation { } } + resolve(): void { + this.analysis.forEach((op, decl) => { + if (op.adapter.resolve !== undefined) { + op.adapter.resolve(decl, op.analysis); + } + }); + } + typeCheck(context: TypeCheckContext): void { this.typeCheckMap.forEach((handler, node) => { if (handler.typeCheck !== undefined) { diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 13867a0b40..a88646bd4b 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -1402,13 +1402,51 @@ describe('ngtsc behavioral tests', () => { declarations: [Cmp, DirA, DirB], }) class Module {} - `); + `); env.driveMain(); const jsContents = env.getContents('test.js'); expect(jsContents).toMatch(/directives: \[DirA,\s+DirB\]/); }); + describe('cycle detection', () => { + it('should detect a simple cycle and use remote component scoping', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + import {NormalComponent} from './cyclic'; + + @Component({ + selector: 'cyclic-component', + template: 'Importing this causes a cycle', + }) + export class CyclicComponent {} + + @NgModule({ + declarations: [NormalComponent, CyclicComponent], + }) + export class Module {} + `); + + env.write('cyclic.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'normal-component', + template: '', + }) + export class NormalComponent {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents) + .toContain( + 'i0.ɵsetComponentScope(NormalComponent, [i1.NormalComponent, CyclicComponent], [])'); + expect(jsContents).not.toContain('/*__PURE__*/ i0.ɵsetComponentScope'); + }); + }); + describe('duplicate local refs', () => { const getComponentScript = (template: string): string => ` import {Component, Directive, NgModule} from '@angular/core'; diff --git a/packages/compiler/src/compiler.ts b/packages/compiler/src/compiler.ts index 5155c670c1..e84e6b8575 100644 --- a/packages/compiler/src/compiler.ts +++ b/packages/compiler/src/compiler.ts @@ -91,6 +91,7 @@ export * from './render3/view/api'; export {BoundAttribute as TmplAstBoundAttribute, BoundEvent as TmplAstBoundEvent, BoundText as TmplAstBoundText, Content as TmplAstContent, Element as TmplAstElement, Node as TmplAstNode, Reference as TmplAstReference, Template as TmplAstTemplate, Text as TmplAstText, TextAttribute as TmplAstTextAttribute, Variable as TmplAstVariable,} from './render3/r3_ast'; export * from './render3/view/t2_api'; export * from './render3/view/t2_binder'; +export {Identifiers as R3Identifiers} from './render3/r3_identifiers'; export {jitExpression} from './render3/r3_jit'; export {R3DependencyMetadata, R3FactoryMetadata, R3ResolvedDependencyType} from './render3/r3_factory'; export {compileInjector, compileNgModule, R3InjectorMetadata, R3NgModuleMetadata} from './render3/r3_module_compiler'; diff --git a/packages/compiler/src/render3/r3_identifiers.ts b/packages/compiler/src/render3/r3_identifiers.ts index 7814cfc2a7..2a52f5619e 100644 --- a/packages/compiler/src/render3/r3_identifiers.ts +++ b/packages/compiler/src/render3/r3_identifiers.ts @@ -147,6 +147,8 @@ export class Identifiers { static defineComponent: o.ExternalReference = {name: 'ɵdefineComponent', moduleName: CORE}; + static setComponentScope: o.ExternalReference = {name: 'ɵsetComponentScope', moduleName: CORE}; + static ComponentDefWithMeta: o.ExternalReference = { name: 'ɵComponentDefWithMeta', moduleName: CORE, diff --git a/packages/core/src/core_render3_private_export.ts b/packages/core/src/core_render3_private_export.ts index 3ff5ca0176..f1b2f6739e 100644 --- a/packages/core/src/core_render3_private_export.ts +++ b/packages/core/src/core_render3_private_export.ts @@ -25,6 +25,7 @@ export { injectAttribute as ɵinjectAttribute, getFactoryOf as ɵgetFactoryOf, getInheritedFactory as ɵgetInheritedFactory, + setComponentScope as ɵsetComponentScope, templateRefExtractor as ɵtemplateRefExtractor, ProvidersFeature as ɵProvidersFeature, InheritDefinitionFeature as ɵInheritDefinitionFeature, diff --git a/packages/core/src/render3/definition.ts b/packages/core/src/render3/definition.ts index 24574a9cff..c5adb32876 100644 --- a/packages/core/src/render3/definition.ts +++ b/packages/core/src/render3/definition.ts @@ -298,6 +298,13 @@ export function defineComponent(componentDefinition: { return def as never; } +export function setComponentScope( + type: ComponentType, directives: Type[], pipes: Type[]): void { + const def = (type.ngComponentDef as ComponentDef); + def.directiveDefs = () => directives.map(extractDirectiveDef); + def.pipeDefs = () => pipes.map(extractPipeDef); +} + export function extractDirectiveDef(type: DirectiveType& ComponentType): DirectiveDef|ComponentDef { const def = getComponentDef(type) || getDirectiveDef(type); diff --git a/packages/core/src/render3/index.ts b/packages/core/src/render3/index.ts index bc57381553..58eab2fe86 100644 --- a/packages/core/src/render3/index.ts +++ b/packages/core/src/render3/index.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import {LifecycleHooksFeature, renderComponent, whenRendered} from './component'; -import {defineBase, defineComponent, defineDirective, defineNgModule, definePipe} from './definition'; +import {defineBase, defineComponent, defineDirective, defineNgModule, definePipe, setComponentScope} from './definition'; import {getComponent, getDirectives, getHostElement, getRenderedText} from './discovery_utils'; import {InheritDefinitionFeature} from './features/inherit_definition_feature'; import {NgOnChangesFeature} from './features/ng_onchanges_feature'; @@ -177,6 +177,7 @@ export { getDirectives, getRenderedText, renderComponent, + setComponentScope, whenRendered, }; diff --git a/packages/core/src/render3/jit/environment.ts b/packages/core/src/render3/jit/environment.ts index aa116cccbe..a5aa778f9a 100644 --- a/packages/core/src/render3/jit/environment.ts +++ b/packages/core/src/render3/jit/environment.ts @@ -113,6 +113,7 @@ export const angularCoreEnv: {[name: string]: Function} = { 'ɵresolveWindow': r3.resolveWindow, 'ɵresolveDocument': r3.resolveDocument, 'ɵresolveBody': r3.resolveBody, + 'ɵsetComponentScope': r3.setComponentScope, 'ɵsanitizeHtml': sanitization.sanitizeHtml, 'ɵsanitizeStyle': sanitization.sanitizeStyle, diff --git a/packages/core/test/bundling/cyclic_import/BUILD.bazel b/packages/core/test/bundling/cyclic_import/BUILD.bazel new file mode 100644 index 0000000000..72de586885 --- /dev/null +++ b/packages/core/test/bundling/cyclic_import/BUILD.bazel @@ -0,0 +1,88 @@ +package(default_visibility = ["//visibility:public"]) + +load("//tools:defaults.bzl", "jasmine_node_test", "ng_module", "ng_rollup_bundle", "ts_library") +load("//tools/symbol-extractor:index.bzl", "js_expected_symbol_test") +load("//tools/http-server:http_server.bzl", "http_server") + +ng_module( + name = "cyclic_import", + srcs = [ + "index.ts", + "trigger.ts", + ], + tags = [ + "ivy-only", + ], + deps = [ + "//packages/core", + ], +) + +ng_rollup_bundle( + name = "bundle", + # TODO(alexeagle): This is inconsistent. + # We try to teach users to always have their workspace at the start of a + # path, to disambiguate from other workspaces. + # Here, the rule implementation is looking in an execroot where the layout + # has an "external" directory for external dependencies. + # This should probably start with "angular/" and let the rule deal with it. + entry_point = "packages/core/test/bundling/cyclic_import/index.js", + tags = [ + "ivy-only", + ], + deps = [ + ":cyclic_import", + "//packages/core", + ], +) + +ts_library( + name = "test_lib", + testonly = True, + srcs = glob(["*_spec.ts"]), + tags = [ + "ivy-only", + ], + deps = [ + "//packages:types", + "//packages/compiler", + "//packages/core/testing", + "//packages/private/testing", + ], +) + +jasmine_node_test( + name = "test", + data = [ + ":bundle", + ":bundle.js", + ":bundle.min.js.br", + ":bundle.min_debug.js", + ], + tags = [ + "ivy-only", + ], + deps = [":test_lib"], +) + +js_expected_symbol_test( + name = "symbol_test", + src = ":bundle.min_debug.js", + golden = ":bundle.golden_symbols.json", + tags = [ + "ivy-aot", + "ivy-only", + ], +) + +http_server( + name = "devserver", + data = [ + "index.html", + ":bundle.min.js", + ":bundle.min_debug.js", + ], + tags = [ + "ivy-only", + ], +) diff --git a/packages/core/test/bundling/cyclic_import/README.md b/packages/core/test/bundling/cyclic_import/README.md new file mode 100644 index 0000000000..e62994c4c9 --- /dev/null +++ b/packages/core/test/bundling/cyclic_import/README.md @@ -0,0 +1,22 @@ +# Purpose + +This test exists to validate that ngtsc, when compiling an application where Ivy would otherwise +require a circular dependency, uses "remote scoping" via the `setComponentScope` function instead. + +# How it works + +There are two files, `index.ts` and `trigger.ts`. `index.ts` contains the NgModule and a simple +component (``). + +`trigger.ts` contains a component `TriggerComponent` that uses `` in its template. Normally, +Ivy would want `DepComponent` to be listed in `TriggerComponent`'s definition. However, this +requires adding an import from `trigger.ts` -> `index.ts`, and there's already an import from +`index.ts` to `trigger.ts` (for the NgModule). + +In this case, ngtsc decides to set the directives in `TriggerComponent`'s definition via a different +mechanism: remote scoping. Alongside the NgModule (in `index.ts`) a call to `setComponentScope` is +generated which sets up `TriggerComponent`'s definition correctly, without introducing any imports. +This call is not tree-shakeable, but does not create a cycle. + +The symbol test here verifies that `setComponentScope` is used, and the e2e spec verifies that the +application works correctly. \ No newline at end of file diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json new file mode 100644 index 0000000000..7c91dda5c3 --- /dev/null +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -0,0 +1,644 @@ +[ + { + "name": "ACTIVE_INDEX" + }, + { + "name": "ANIMATION_PROP_PREFIX" + }, + { + "name": "BINDING_INDEX" + }, + { + "name": "BLOOM_MASK" + }, + { + "name": "CLEAN_PROMISE" + }, + { + "name": "CONTAINER_INDEX" + }, + { + "name": "CONTEXT" + }, + { + "name": "ChangeDetectionStrategy" + }, + { + "name": "DECLARATION_VIEW" + }, + { + "name": "DepComponent" + }, + { + "name": "EMPTY_ARRAY" + }, + { + "name": "EMPTY_OBJ" + }, + { + "name": "EmptyErrorImpl" + }, + { + "name": "FLAGS" + }, + { + "name": "FactoryPrototype" + }, + { + "name": "HEADER_OFFSET" + }, + { + "name": "HOST" + }, + { + "name": "HOST_NODE" + }, + { + "name": "INJECTOR" + }, + { + "name": "INJECTOR_BLOOM_PARENT_SIZE" + }, + { + "name": "LCONTAINER_LENGTH" + }, + { + "name": "MONKEY_PATCH_KEY_NAME" + }, + { + "name": "Module" + }, + { + "name": "NATIVE" + }, + { + "name": "NEXT" + }, + { + "name": "NG_COMPONENT_DEF" + }, + { + "name": "NG_DIRECTIVE_DEF" + }, + { + "name": "NG_ELEMENT_ID" + }, + { + "name": "NG_PIPE_DEF" + }, + { + "name": "NG_PROJECT_AS_ATTR_NAME" + }, + { + "name": "NG_TEMPLATE_SELECTOR" + }, + { + "name": "NO_CHANGE" + }, + { + "name": "NO_PARENT_INJECTOR" + }, + { + "name": "NodeInjectorFactory" + }, + { + "name": "ObjectUnsubscribedErrorImpl" + }, + { + "name": "PARENT" + }, + { + "name": "PARENT_INJECTOR" + }, + { + "name": "QUERIES" + }, + { + "name": "RENDERER" + }, + { + "name": "RENDERER_FACTORY" + }, + { + "name": "RendererStyleFlags3" + }, + { + "name": "SANITIZER" + }, + { + "name": "TAIL" + }, + { + "name": "TVIEW" + }, + { + "name": "TriggerComponent" + }, + { + "name": "UnsubscriptionErrorImpl" + }, + { + "name": "VIEWS" + }, + { + "name": "ViewEncapsulation" + }, + { + "name": "__self" + }, + { + "name": "__values" + }, + { + "name": "__window" + }, + { + "name": "_currentNamespace" + }, + { + "name": "_global" + }, + { + "name": "_renderCompCount" + }, + { + "name": "addComponentLogic" + }, + { + "name": "addToViewTree" + }, + { + "name": "allocStylingContext" + }, + { + "name": "appendChild" + }, + { + "name": "attachPatchData" + }, + { + "name": "baseResolveDirective" + }, + { + "name": "bloomAdd" + }, + { + "name": "cacheMatchingLocalNames" + }, + { + "name": "callHooks" + }, + { + "name": "checkNoChangesMode" + }, + { + "name": "checkView" + }, + { + "name": "componentRefresh" + }, + { + "name": "createDirectivesAndLocals" + }, + { + "name": "createEmptyStylingContext" + }, + { + "name": "createLView" + }, + { + "name": "createNodeAtIndex" + }, + { + "name": "createRootComponent" + }, + { + "name": "createRootComponentView" + }, + { + "name": "createRootContext" + }, + { + "name": "createTNode" + }, + { + "name": "createTView" + }, + { + "name": "createTextNode" + }, + { + "name": "createViewBlueprint" + }, + { + "name": "decreaseElementDepthCount" + }, + { + "name": "defaultScheduler" + }, + { + "name": "defineComponent" + }, + { + "name": "defineInjector" + }, + { + "name": "defineNgModule" + }, + { + "name": "diPublicInInjector" + }, + { + "name": "domRendererFactory3" + }, + { + "name": "element" + }, + { + "name": "elementCreate" + }, + { + "name": "elementEnd" + }, + { + "name": "elementStart" + }, + { + "name": "enterView" + }, + { + "name": "executeHooks" + }, + { + "name": "executeInitHooks" + }, + { + "name": "executeViewQueryFn" + }, + { + "name": "extractDirectiveDef" + }, + { + "name": "extractPipeDef" + }, + { + "name": "findAttrIndexInNode" + }, + { + "name": "findDirectiveMatches" + }, + { + "name": "firstTemplatePass" + }, + { + "name": "generateExpandoInstructionBlock" + }, + { + "name": "generateInitialInputs" + }, + { + "name": "generatePropertyAliases" + }, + { + "name": "getBeforeNodeForView" + }, + { + "name": "getBindingsEnabled" + }, + { + "name": "getCheckNoChangesMode" + }, + { + "name": "getClosureSafeProperty" + }, + { + "name": "getComponentDef" + }, + { + "name": "getComponentViewByIndex" + }, + { + "name": "getContainerRenderParent" + }, + { + "name": "getDirectiveDef" + }, + { + "name": "getElementDepthCount" + }, + { + "name": "getFirstTemplatePass" + }, + { + "name": "getHighestElementOrICUContainer" + }, + { + "name": "getHostNative" + }, + { + "name": "getInitialClassNameValue" + }, + { + "name": "getInjectorIndex" + }, + { + "name": "getIsParent" + }, + { + "name": "getLContainer" + }, + { + "name": "getLView" + }, + { + "name": "getLViewChild" + }, + { + "name": "getNativeAnchorNode" + }, + { + "name": "getNativeByTNode" + }, + { + "name": "getNodeInjectable" + }, + { + "name": "getOrCreateNodeInjectorForNode" + }, + { + "name": "getOrCreateTView" + }, + { + "name": "getParentInjectorIndex" + }, + { + "name": "getParentInjectorLocation" + }, + { + "name": "getParentInjectorView" + }, + { + "name": "getParentInjectorViewOffset" + }, + { + "name": "getPipeDef" + }, + { + "name": "getPreviousOrParentTNode" + }, + { + "name": "getRenderFlags" + }, + { + "name": "getRenderParent" + }, + { + "name": "getRootContext" + }, + { + "name": "getRootView" + }, + { + "name": "getStylingContext" + }, + { + "name": "getTNode" + }, + { + "name": "hasClassInput" + }, + { + "name": "hasParentInjector" + }, + { + "name": "hasStyling" + }, + { + "name": "hasTagAndTypeMatch" + }, + { + "name": "includeViewProviders" + }, + { + "name": "increaseElementDepthCount" + }, + { + "name": "initNodeFlags" + }, + { + "name": "initializeStaticContext" + }, + { + "name": "initializeTNodeInputs" + }, + { + "name": "insertBloom" + }, + { + "name": "instantiateAllDirectives" + }, + { + "name": "instantiateRootComponent" + }, + { + "name": "invertObject" + }, + { + "name": "invokeDirectivesHostBindings" + }, + { + "name": "isAnimationProp" + }, + { + "name": "isComponentDef" + }, + { + "name": "isContentQueryHost" + }, + { + "name": "isCreationMode" + }, + { + "name": "isCssClassMatching" + }, + { + "name": "isFactory" + }, + { + "name": "isNodeMatchingSelector" + }, + { + "name": "isNodeMatchingSelectorList" + }, + { + "name": "isPositive" + }, + { + "name": "isProceduralRenderer" + }, + { + "name": "isRootView" + }, + { + "name": "isStylingContext" + }, + { + "name": "leaveView" + }, + { + "name": "locateHostElement" + }, + { + "name": "namespaceHTML" + }, + { + "name": "nativeAppendChild" + }, + { + "name": "nativeAppendOrInsertBefore" + }, + { + "name": "nativeInsertBefore" + }, + { + "name": "nativeParentNode" + }, + { + "name": "nextNgElementId" + }, + { + "name": "noSideEffects" + }, + { + "name": "postProcessBaseDirective" + }, + { + "name": "postProcessDirective" + }, + { + "name": "queueComponentIndexForCheck" + }, + { + "name": "readClassValueFromTNode" + }, + { + "name": "readElementValue" + }, + { + "name": "readPatchedData" + }, + { + "name": "readPatchedLView" + }, + { + "name": "refreshChildComponents" + }, + { + "name": "refreshContentQueries" + }, + { + "name": "refreshDescendantViews" + }, + { + "name": "refreshDynamicEmbeddedViews" + }, + { + "name": "registerPostOrderHooks" + }, + { + "name": "registerPreOrderHooks" + }, + { + "name": "renderComponent" + }, + { + "name": "renderComponentOrTemplate" + }, + { + "name": "renderEmbeddedTemplate" + }, + { + "name": "renderInitialStylesAndClasses" + }, + { + "name": "renderInitialStylingValues" + }, + { + "name": "renderStringify" + }, + { + "name": "resetComponentState" + }, + { + "name": "resolveDirectives" + }, + { + "name": "saveNameToExportMap" + }, + { + "name": "saveResolvedLocalsInData" + }, + { + "name": "setBindingRoot" + }, + { + "name": "setClass" + }, + { + "name": "setComponentScope" + }, + { + "name": "setCurrentDirectiveDef" + }, + { + "name": "setCurrentViewQueryIndex" + }, + { + "name": "setFirstTemplatePass" + }, + { + "name": "setHostBindings" + }, + { + "name": "setIncludeViewProviders" + }, + { + "name": "setInjectImplementation" + }, + { + "name": "setInputsForProperty" + }, + { + "name": "setInputsFromAttrs" + }, + { + "name": "setIsParent" + }, + { + "name": "setPreviousOrParentTNode" + }, + { + "name": "setStyle" + }, + { + "name": "setTNodeAndViewData" + }, + { + "name": "setUpAttributes" + }, + { + "name": "syncViewWithBlueprint" + }, + { + "name": "text" + }, + { + "name": "throwMultipleComponentError" + }, + { + "name": "tickRootContext" + }, + { + "name": "viewAttached" + } +] \ No newline at end of file diff --git a/packages/core/test/bundling/cyclic_import/index.html b/packages/core/test/bundling/cyclic_import/index.html new file mode 100644 index 0000000000..ab44f4f0aa --- /dev/null +++ b/packages/core/test/bundling/cyclic_import/index.html @@ -0,0 +1,31 @@ + + + + + Angular Cyclic Import Example + + + + + + + + + \ No newline at end of file diff --git a/packages/core/test/bundling/cyclic_import/index.ts b/packages/core/test/bundling/cyclic_import/index.ts new file mode 100644 index 0000000000..b3c2b5160c --- /dev/null +++ b/packages/core/test/bundling/cyclic_import/index.ts @@ -0,0 +1,26 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Component, NgModule, ɵrenderComponent as renderComponent} from '@angular/core'; + +import {TriggerComponent} from './trigger'; + +@Component({ + selector: 'dep', + template: 'dep', +}) +export class DepComponent { +} + +@NgModule({ + declarations: [DepComponent, TriggerComponent], +}) +export class Module { +} + +renderComponent(TriggerComponent); diff --git a/packages/core/test/bundling/cyclic_import/integration_spec.ts b/packages/core/test/bundling/cyclic_import/integration_spec.ts new file mode 100644 index 0000000000..e1a2d4f318 --- /dev/null +++ b/packages/core/test/bundling/cyclic_import/integration_spec.ts @@ -0,0 +1,41 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import '@angular/compiler'; +import {withBody} from '@angular/private/testing'; +import * as fs from 'fs'; +import * as path from 'path'; + +const UTF8 = { + encoding: 'utf-8' +}; +const PACKAGE = 'angular/packages/core/test/bundling/cyclic_import'; + +describe('treeshaking with uglify', () => { + + let content: string; + const contentPath = require.resolve(path.join(PACKAGE, 'bundle.min_debug.js')); + beforeAll(() => { content = fs.readFileSync(contentPath, UTF8); }); + + describe('functional test in domino', () => { + it('should render hello world when not minified', withBody('', () => { + require(path.join(PACKAGE, 'bundle.js')); + expect(document.body.textContent).toEqual('dep'); + })); + + it('should render hello world when debug minified', withBody('', () => { + require(path.join(PACKAGE, 'bundle.min_debug.js')); + expect(document.body.textContent).toEqual('dep'); + })); + + it('should render hello world when fully minified', withBody('', () => { + require(path.join(PACKAGE, 'bundle.min.js')); + expect(document.body.textContent).toEqual('dep'); + })); + }); +}); diff --git a/packages/core/test/bundling/cyclic_import/trigger.ts b/packages/core/test/bundling/cyclic_import/trigger.ts new file mode 100644 index 0000000000..b51ba10cc5 --- /dev/null +++ b/packages/core/test/bundling/cyclic_import/trigger.ts @@ -0,0 +1,16 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Component} from '@angular/core'; + +@Component({ + selector: 'trigger', + template: '', +}) +export class TriggerComponent { +} diff --git a/packages/core/test/render3/ivy/jit_spec.ts b/packages/core/test/render3/ivy/jit_spec.ts index 5978a05fab..acebef453d 100644 --- a/packages/core/test/render3/ivy/jit_spec.ts +++ b/packages/core/test/render3/ivy/jit_spec.ts @@ -7,7 +7,7 @@ */ import 'reflect-metadata'; -import {ElementRef, QueryList} from '@angular/core'; +import {ElementRef, QueryList, ɵsetComponentScope as setComponentScope} from '@angular/core'; import {Injectable} from '@angular/core/src/di/injectable'; import {inject, setCurrentInjector} from '@angular/core/src/di/injector_compatibility'; import {InjectorDef, defineInjectable} from '@angular/core/src/di/interface/defs';