honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(compiler-cli): setComponentScope should only list used components/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
This commit is contained in:
Alex Rickabaugh 2020-11-12 13:44:24 -08:00 committed by atscott
parent 25d6fcacc4
commit c59f401f9a
7 changed files with 95 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
packages/compiler-cli/src/ngtsc/annotations/src/component.ts
View File

@ -510,16 +510,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),
});
@ -557,7 +559,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));
}
}

14
packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
View File

@ -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)!;

17
packages/compiler-cli/src/ngtsc/scope/src/api.ts
View File

@ -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';
@ -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
packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts
View File

@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {ClassDeclaration} from '../../reflection';
import {RemoteScope} from './api';
import {LocalModuleScope} from './local';
/**
@ -13,7 +14,14 @@ import {LocalModuleScope} from './local';
*/
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;
}
/**
@ -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;

18
packages/compiler-cli/src/ngtsc/scope/src/local.ts
View File

@ -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';
@ -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.
@ -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});
}
/**

2
packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
View File

@ -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

45
packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
View File

@ -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');
});
@ -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', () => {