Skip to content

Commit

Permalink
fix(compiler-cli): setComponentScope should only list used components…
Browse files Browse the repository at this point in the history
…/pipes (#39662)

ngtsc will avoid emitting generated imports that would create an import
cycle in the user's program. The main way such imports can arise is when
a component would ordinarily reference its dependencies in its component
definition `directiveDefs` and `pipeDefs`. This requires adding imports,
which run the risk of creating a cycle.

When ngtsc detects that adding such an import would cause this to occur, it
instead falls back on a strategy called "remote scoping", where a side-
effectful call to `setComponentScope` in the component's NgModule file is
used to patch `directiveDefs` and `pipeDefs` onto the component. Since the
NgModule file already imports all of the component's dependencies (to
declare them in the NgModule), this approach does not risk adding a cycle.
It has several large downsides, however:

1. it breaks under `sideEffects: false` logic in bundlers including the CLI
2. it breaks tree-shaking for the given component and its dependencies

See this doc for further details: https://hackmd.io/Odw80D0pR6yfsOjg_7XCJg?view

In particular, the impact on tree-shaking was exacerbated by the naive logic
ngtsc used to employ here. When this feature was implemented, at the time of
generating the side-effectful `setComponentScope` call, the compiler did not
know which of the component's declared dependencies were actually used in
its template. This meant that unlike the generation of `directiveDefs` in
the component definition itself, `setComponentScope` calls had to list the
_entire_ compilation scope of the component's NgModule, including directives
and pipes which were not actually used in the template. This made the tree-
shaking impact much worse, since if the component's NgModule made use of any
shared NgModules (e.g. `CommonModule`), every declaration therein would
become un-treeshakable.

Today, ngtsc does have the information on which directives/pipes are
actually used in the template, but this was not being used during the remote
scoping operation. This commit modifies remote scoping to take advantage of
the extra context and only list used dependencies in `setComponentScope`
calls, which should ameliorate the tree-shaking impact somewhat.

PR Close #39662
  • Loading branch information
alxhub authored and atscott committed Nov 13, 2020
1 parent f1d8717 commit 8d317df
Show file tree
Hide file tree
Showing 7 changed files with 95 additions and 26 deletions.
7 changes: 5 additions & 2 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
Expand Up @@ -503,16 +503,18 @@ export class ComponentDecoratorHandler implements
return {
selector: directive.selector,
expression: this.refEmitter.emit(directive.ref, context),
ref: directive.ref,
};
});

const usedPipes: {pipeName: string, expression: Expression}[] = [];
const usedPipes: {ref: Reference, pipeName: string, expression: Expression}[] = [];
for (const pipeName of bound.getUsedPipes()) {
if (!pipes.has(pipeName)) {
continue;
}
const pipe = pipes.get(pipeName)!;
usedPipes.push({
ref: pipe,
pipeName,
expression: this.refEmitter.emit(pipe, context),
});
Expand Down Expand Up @@ -550,7 +552,8 @@ export class ComponentDecoratorHandler implements
// 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);
this.scopeRegistry.setComponentRemoteScope(
node, usedDirectives.map(dir => dir.ref), usedPipes.map(pipe => pipe.ref));
}
}

Expand Down
14 changes: 5 additions & 9 deletions packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
Expand Up @@ -387,15 +387,11 @@ export class NgModuleDecoratorHandler implements
}
const context = getSourceFile(node);
for (const decl of analysis.declarations) {
if (this.scopeRegistry.getRequiresRemoteScope(decl.node)) {
const scope = this.scopeRegistry.getScopeOfModule(ts.getOriginalNode(node) as typeof node);
if (scope === null || scope === 'error') {
continue;
}

const directives = scope.compilation.directives.map(
directive => this.refEmitter.emit(directive.ref, context));
const pipes = scope.compilation.pipes.map(pipe => this.refEmitter.emit(pipe.ref, context));
const remoteScope = this.scopeRegistry.getRemoteScope(decl.node);
if (remoteScope !== null) {
const directives =
remoteScope.directives.map(directive => this.refEmitter.emit(directive, context));
const pipes = remoteScope.pipes.map(pipe => this.refEmitter.emit(pipe, context));
const directiveArray = new LiteralArrayExpr(directives);
const pipesArray = new LiteralArrayExpr(pipes);
const declExpr = this.refEmitter.emit(decl, context)!;
Expand Down
17 changes: 17 additions & 0 deletions packages/compiler-cli/src/ngtsc/scope/src/api.ts
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Reference} from '../../imports';
import {DirectiveMeta, PipeMeta} from '../../metadata';
import {ClassDeclaration} from '../../reflection';

Expand Down Expand Up @@ -40,3 +41,19 @@ export interface ExportScope {
*/
exported: ScopeData;
}

/**
* A resolved scope for a given component that cannot be set locally in the component definition,
* and must be set via remote scoping call in the component's NgModule file.
*/
export interface RemoteScope {
/**
* Those directives used by the component that requires this scope to be set remotely.
*/
directives: Reference[];

/**
* Those pipes used by the component that requires this scope to be set remotely.
*/
pipes: Reference[];
}
18 changes: 13 additions & 5 deletions packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts
Expand Up @@ -6,14 +6,22 @@
* found in the LICENSE file at https://angular.io/license
*/
import {ClassDeclaration} from '../../reflection';
import {RemoteScope} from './api';
import {LocalModuleScope} from './local';

/**
* Read information about the compilation scope of components.
*/
export interface ComponentScopeReader {
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error';
getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null;

/**
* Get the `RemoteScope` required for this component, if any.
*
* If the component requires remote scoping, then retrieve the directives/pipes registered for
* that component. If remote scoping is not required (the common case), returns `null`.
*/
getRemoteScope(clazz: ClassDeclaration): RemoteScope|null;
}

/**
Expand All @@ -36,11 +44,11 @@ export class CompoundComponentScopeReader implements ComponentScopeReader {
return null;
}

getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null {
getRemoteScope(clazz: ClassDeclaration): RemoteScope|null {
for (const reader of this.readers) {
const requiredScoping = reader.getRequiresRemoteScope(clazz);
if (requiredScoping !== null) {
return requiredScoping;
const remoteScope = reader.getRemoteScope(clazz);
if (remoteScope !== null) {
return remoteScope;
}
}
return null;
Expand Down
18 changes: 10 additions & 8 deletions packages/compiler-cli/src/ngtsc/scope/src/local.ts
Expand Up @@ -15,7 +15,7 @@ import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta}
import {ClassDeclaration, DeclarationNode} from '../../reflection';
import {identifierOfNode, nodeNameForError} from '../../util/src/typescript';

import {ExportScope, ScopeData} from './api';
import {ExportScope, RemoteScope, ScopeData} from './api';
import {ComponentScopeReader} from './component_scope';
import {DtsModuleScopeResolver} from './dependency';

Expand Down Expand Up @@ -96,14 +96,14 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
private cache = new Map<ClassDeclaration, LocalModuleScope|undefined|null>();

/**
* Tracks whether a given component requires "remote scoping".
* Tracks the `RemoteScope` for components requiring "remote scoping".
*
* Remote scoping is when the set of directives which apply to a given component is set in the
* NgModule's file instead of directly on the component def (which is sometimes needed to get
* around cyclic import issues). This is not used in calculation of `LocalModuleScope`s, but is
* tracked here for convenience.
*/
private remoteScoping = new Set<ClassDeclaration>();
private remoteScoping = new Map<ClassDeclaration, RemoteScope>();

/**
* Tracks errors accumulated in the processing of scopes for each module declaration.
Expand Down Expand Up @@ -452,15 +452,17 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
/**
* Check whether a component requires remote scoping.
*/
getRequiresRemoteScope(node: ClassDeclaration): boolean {
return this.remoteScoping.has(node);
getRemoteScope(node: ClassDeclaration): RemoteScope|null {
return this.remoteScoping.has(node) ? this.remoteScoping.get(node)! : null;
}

/**
* Set a component as requiring remote scoping.
* Set a component as requiring remote scoping, with the given directives and pipes to be
* registered remotely.
*/
setComponentAsRequiringRemoteScoping(node: ClassDeclaration): void {
this.remoteScoping.add(node);
setComponentRemoteScope(node: ClassDeclaration, directives: Reference[], pipes: Reference[]):
void {
this.remoteScoping.set(node, {directives, pipes});
}

/**
Expand Down
Expand Up @@ -418,7 +418,7 @@ export function setup(targets: TypeCheckingTarget[], overrides: {
}

const fakeScopeReader: ComponentScopeReader = {
getRequiresRemoteScope() {
getRemoteScope(): null {
return null;
},
// If there is a module with [className] + 'Module' in the same source file, that will be
Expand Down
45 changes: 44 additions & 1 deletion packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Expand Up @@ -4597,7 +4597,7 @@ runInEachFileSystem(os => {
const jsContents = env.getContents('test.js');
expect(jsContents)
.toMatch(
/i\d\.ɵɵsetComponentScope\(NormalComponent,\s+\[NormalComponent,\s+CyclicComponent\],\s+\[\]\)/);
/i\d\.ɵɵsetComponentScope\(NormalComponent,\s+\[CyclicComponent\],\s+\[\]\)/);
expect(jsContents).not.toContain('/*__PURE__*/ i0.ɵɵsetComponentScope');
});

Expand Down Expand Up @@ -4666,6 +4666,49 @@ runInEachFileSystem(os => {
const jsContents = env.getContents('test.js');
expect(jsContents).not.toContain('setComponentScope');
});

it('should only pass components actually used to setComponentScope', () => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
import {NormalComponent} from './cyclic';
import {OtherComponent} from './other';
@Component({
selector: 'cyclic-component',
template: 'Importing this causes a cycle',
})
export class CyclicComponent {}
@NgModule({
declarations: [NormalComponent, CyclicComponent, OtherComponent],
})
export class Module {}
`);

env.write('cyclic.ts', `
import {Component} from '@angular/core';
@Component({
selector: 'normal-component',
template: '<cyclic-component></cyclic-component>',
})
export class NormalComponent {}
`);

env.write('other.ts', `
import {Component} from '@angular/core';
@Component({
selector: 'other-component',
template: 'An unused other component',
})
export class OtherComponent {}
`);

env.driveMain();
const jsContents = env.getContents('test.js');
expect(jsContents).not.toMatch(/i\d\.ɵɵsetComponentScope\([^)]*OtherComponent[^)]*\)/);
});
});

describe('local refs', () => {
Expand Down

0 comments on commit 8d317df

Please sign in to comment.