From 3e569767e31d29096e6b683e9a1cdb3e1d5e6ae9 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 19 Mar 2019 13:10:51 -0700 Subject: [PATCH] fix(ivy): avoid remote scoping if it's not actually required (#29404) Currently, ngtsc decides to use remote scoping if the compilation of a component may create a cyclic import. This happens if there are two components in a scope (say, A and B) and A directly uses B. During compilation of B ngtsc will then note that if B were to use A, a cycle would be generated, and so it will opt to use remote scoping for B. ngtsc already uses the R3TargetBinder to correctly track the imports that are actually required, for future cycle tracking. This commit expands that usage to not trigger remote scoping unless B actually does consume A in its template. PR Close #29404 --- .../src/ngtsc/annotations/src/component.ts | 83 +++++++++++++------ .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 33 ++++++++ 2 files changed, 92 insertions(+), 24 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index a722ae4879..6504c29238 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -322,7 +322,27 @@ 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. + // + // First it needs to be determined if actually importing the directives/pipes used in the + // template would create a cycle. Currently ngtsc refuses to generate cycles, so an option + // known as "remote scoping" is used if a cycle would be created. In remote scoping, the + // module file sets the directives/pipes on the ngComponentDef of the component, without + // requiring new imports (but also in a way that breaks tree shaking). + // + // Determining this is challenging, because the TemplateDefinitionBuilder is responsible for + // matching directives and pipes in the template; however, that doesn't run until the actual + // compile() step. It's not possible to run template compilation sooner as it requires the + // ConstantPool for the overall file being compiled (which isn't available until the transform + // step). + // + // Instead, directives/pipes are matched independently here, using the R3TargetBinder. This is + // an alternative implementation of template matching which is used for template type-checking + // and will eventually replace matching in the TemplateDefinitionBuilder. + + // Set up the R3TargetBinder, as well as a 'directives' array and a 'pipes' map that are later + // fed to the TemplateDefinitionBuilder. First, a SelectorMatcher is constructed to match + // directives that are in scope. const matcher = new SelectorMatcher(); const directives: {selector: string, expression: Expression}[] = []; @@ -330,7 +350,6 @@ export class ComponentDecoratorHandler implements 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(); @@ -338,36 +357,52 @@ export class ComponentDecoratorHandler implements pipes.set(pipe.name, this.refEmitter.emit(pipe.ref, context)); } - // 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 cycleDetected = directives.some(dir => this._isCyclicImport(dir.expression, context)) || - Array.from(pipes.values()).some(pipe => this._isCyclicImport(pipe, context)); + // Next, the component template AST is bound using the R3TargetBinder. This produces an + // BoundTarget, which is similar to a ts.TypeChecker. + const binder = new R3TargetBinder(matcher); + const bound = binder.bind({template: metadata.template.nodes}); + + // The BoundTarget knows which directives and pipes matched the template. + const usedDirectives = bound.getUsedDirectives(); + const usedPipes = bound.getUsedPipes().map(name => pipes.get(name) !); + + // Scan through the directives/pipes actually used in the template and check whether any + // import which needs to be generated would create a cycle. + const cycleDetected = + usedDirectives.some(dir => this._isCyclicImport(dir.expression, context)) || + usedPipes.some(pipe => this._isCyclicImport(pipe, context)); + if (!cycleDetected) { + // No cycle was detected. Record the imports that need to be created in the cycle detector + // so that future cyclic import checks consider their production. + for (const {expression} of usedDirectives) { + this._recordSyntheticImport(expression, context); + } + for (const pipe of usedPipes) { + this._recordSyntheticImport(pipe, context); + } + + // Check whether the directive/pipe arrays in ngComponentDef need to be wrapped in closures. + // This is required if any directive/pipe reference is to a declaration in the same file but + // declared after this component. const wrapDirectivesAndPipesInClosure = - directives.some( + usedDirectives.some( dir => isExpressionForwardReference(dir.expression, node.name, context)) || - Array.from(pipes.values()) - .some(pipe => isExpressionForwardReference(pipe, node.name, context)); + usedPipes.some(pipe => isExpressionForwardReference(pipe, node.name, context)); + + // Actual compilation still uses the full scope, not the narrowed scope determined by + // R3TargetBinder. This is a hedge against potential issues with the R3TargetBinder - right + // now the TemplateDefinitionBuilder is the "source of truth" for which directives/pipes are + // actually used (though the two should agree perfectly). + // + // TODO(alxhub): switch TemplateDefinitionBuilder over to using R3TargetBinder directly. metadata.directives = directives; metadata.pipes = pipes; metadata.wrapDirectivesAndPipesInClosure = wrapDirectivesAndPipesInClosure; - for (const dir of directives) { - this._recordSyntheticImport(dir.expression, context); - } - pipes.forEach((pipe: Expression) => 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 { + // Declaring the directiveDefs/pipeDefs arrays directly would require imports that would + // create a cycle. Instead, mark this component as requiring remote scoping, so that the + // NgModule file will take care of setting the directives for the component. this.scopeRegistry.setComponentAsRequiringRemoteScoping(node); } } diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index d4f5ea8a21..8f9521ccbe 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -2463,6 +2463,39 @@ describe('ngtsc behavioral tests', () => { expect(aJsContents).toMatch(/import \* as i\d? from ".\/b"/); expect(bJsContents).not.toMatch(/import \* as i\d? from ".\/a"/); }); + + it('should not detect a potential cycle if it doesn\'t actually happen', () => { + 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: 'does not use a-cmp', + }) + export class BCmp {} + `); + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).not.toContain('setComponentScope'); + }); }); describe('multiple local refs', () => {