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

fix(ivy): prevent ngcc from inadvertently dropping Ivy definitions (#27159) 

A surprising interaction with the MagicString library caused inserted
Ivy definitions to be dropped during the removal of decorators, iff all
decorators on the class could be removed. In that case, the removal
location corresponds with the exact location where Ivy definitions were
inserted into.

This commit moves the removal of decorators to occur before Ivy
definitions are inserted. This effectively avoids the problem, as later
inserted text fragments will be retained by MagicString.

PR Close #27159
This commit is contained in:
JoostK 2018-11-18 21:37:30 +01:00 committed by Miško Hevery
parent 4df82bdbc1
commit f4a797db24
4 changed files with 38 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
packages/compiler-cli/src/ngcc/src/rendering/esm_renderer.ts
View File

@ -10,8 +10,7 @@ import MagicString from 'magic-string';
import * as ts from 'typescript';
import {NgccReflectionHost, POST_R3_MARKER, PRE_R3_MARKER, SwitchableVariableDeclaration} from '../host/ngcc_host';
import {CompiledClass} from '../analysis/decoration_analyzer';
import {ExportInfo} from '../analysis/private_declarations_analyzer';
import {Renderer, stripExtension} from './renderer';
import {RedundantDecoratorMap, Renderer, stripExtension} from './renderer';
import {EntryPointBundle} from '../packages/entry_point_bundle';
export class EsmRenderer extends Renderer {
@ -71,7 +70,7 @@ export class EsmRenderer extends Renderer {
/**
* Remove static decorator properties from classes
*/
removeDecorators(output: MagicString, decoratorsToRemove: Map<ts.Node, ts.Node[]>): void {
removeDecorators(output: MagicString, decoratorsToRemove: RedundantDecoratorMap): void {
decoratorsToRemove.forEach((nodesToRemove, containerNode) => {
if (ts.isArrayLiteralExpression(containerNode)) {
const items = containerNode.elements;

49
packages/compiler-cli/src/ngcc/src/rendering/renderer.ts
View File

@ -49,6 +49,14 @@ interface DtsClassInfo {
compilation: CompileResult[];
}
/**
* The collected decorators that have become redundant after the compilation
* of Ivy static fields. The map is keyed by the container node, such that we
* can tell if we should remove the entire decorator property
*/
export type RedundantDecoratorMap = Map<ts.Node, ts.Node[]>;
export const RedundantDecoratorMap = Map;
/**
* A base-class for rendering an `AnalyzedFile`.
*
@ -113,12 +121,15 @@ export abstract class Renderer {
if (compiledFile) {
const importManager = new NgccImportManager(this.bundle.isFlat, this.isCore, IMPORT_PREFIX);
const decoratorsToRemove = new Map<ts.Node, ts.Node[]>();
// TODO: remove constructor param metadata and property decorators (we need info from the
// handlers to do this)
const decoratorsToRemove = this.computeDecoratorsToRemove(compiledFile.compiledClasses);
this.removeDecorators(outputText, decoratorsToRemove);
compiledFile.compiledClasses.forEach(clazz => {
const renderedDefinition = renderDefinitions(compiledFile.sourceFile, clazz, importManager);
this.addDefinitions(outputText, clazz, renderedDefinition);
this.trackDecorators(clazz.decorators, decoratorsToRemove);
});
this.addConstants(
@ -129,10 +140,6 @@ export abstract class Renderer {
this.addImports(
outputText, importManager.getAllImports(
compiledFile.sourceFile.fileName, this.bundle.src.r3SymbolsFile));
// TODO: remove constructor param metadata and property decorators (we need info from the
// handlers to do this)
this.removeDecorators(outputText, decoratorsToRemove);
}
// Add exports to the entry-point file
@ -190,25 +197,31 @@ export abstract class Renderer {
protected abstract addDefinitions(
output: MagicString, compiledClass: CompiledClass, definitions: string): void;
protected abstract removeDecorators(
output: MagicString, decoratorsToRemove: Map<ts.Node, ts.Node[]>): void;
output: MagicString, decoratorsToRemove: RedundantDecoratorMap): void;
protected abstract rewriteSwitchableDeclarations(
outputText: MagicString, sourceFile: ts.SourceFile,
declarations: SwitchableVariableDeclaration[]): void;
/**
* Add the decorator nodes that are to be removed to a map
* So that we can tell if we should remove the entire decorator property
* From the given list of classes, computes a map of decorators that should be removed.
* The decorators to remove are keyed by their container node, such that we can tell if
* we should remove the entire decorator property.
* @param classes The list of classes that may have decorators to remove.
* @returns A map of decorators to remove, keyed by their container node.
*/
protected trackDecorators(decorators: Decorator[], decoratorsToRemove: Map<ts.Node, ts.Node[]>):
void {
decorators.forEach(dec => {
const decoratorArray = dec.node.parent !;
if (!decoratorsToRemove.has(decoratorArray)) {
decoratorsToRemove.set(decoratorArray, [dec.node]);
} else {
decoratorsToRemove.get(decoratorArray) !.push(dec.node);
}
protected computeDecoratorsToRemove(classes: CompiledClass[]): RedundantDecoratorMap {
const decoratorsToRemove = new RedundantDecoratorMap();
classes.forEach(clazz => {
clazz.decorators.forEach(dec => {
const decoratorArray = dec.node.parent !;
if (!decoratorsToRemove.has(decoratorArray)) {
decoratorsToRemove.set(decoratorArray, [dec.node]);
} else {
decoratorsToRemove.get(decoratorArray) !.push(dec.node);
}
});
});
return decoratorsToRemove;
}
/**

2
packages/compiler-cli/src/ngcc/test/rendering/esm5_renderer_spec.ts
View File

@ -315,10 +315,12 @@ SOME DEFINITION TEXT
const decoratorsToRemove = new Map<ts.Node, ts.Node[]>();
decoratorsToRemove.set(decorator.node.parent !, [decorator.node]);
renderer.removeDecorators(output, decoratorsToRemove);
renderer.addDefinitions(output, compiledClass, 'SOME DEFINITION TEXT');
expect(output.toString()).toContain(`{ type: Directive, args: [{ selector: '[a]' }] },`);
expect(output.toString()).toContain(`{ type: OtherA }`);
expect(output.toString()).toContain(`{ type: Directive, args: [{ selector: '[b]' }] }`);
expect(output.toString()).toContain(`{ type: OtherB }`);
expect(output.toString()).toContain(`function C() {}\nSOME DEFINITION TEXT\n return C;`);
expect(output.toString()).not.toContain(`C.decorators = [
{ type: Directive, args: [{ selector: '[c]' }] },
];`);

6
packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts
View File

@ -14,7 +14,7 @@ import {NgccReferencesRegistry} from '../../src/analysis/ngcc_references_registr
import {PrivateDeclarationsAnalyzer} from '../../src/analysis/private_declarations_analyzer';
import {SwitchMarkerAnalyzer} from '../../src/analysis/switch_marker_analyzer';
import {Esm2015ReflectionHost} from '../../src/host/esm2015_host';
import {Renderer} from '../../src/rendering/renderer';
import {RedundantDecoratorMap, Renderer} from '../../src/rendering/renderer';
import {EntryPointBundle} from '../../src/packages/entry_point_bundle';
import {makeTestEntryPointBundle} from '../helpers/utils';
@ -37,7 +37,7 @@ class TestRenderer extends Renderer {
addDefinitions(output: MagicString, compiledClass: CompiledClass, definitions: string) {
output.prepend('\n// ADD DEFINITIONS\n');
}
removeDecorators(output: MagicString, decoratorsToRemove: Map<ts.Node, ts.Node[]>) {
removeDecorators(output: MagicString, decoratorsToRemove: RedundantDecoratorMap) {
output.prepend('\n// REMOVE DECORATORS\n');
}
rewriteSwitchableDeclarations(output: MagicString, sourceFile: ts.SourceFile): void {
@ -97,7 +97,7 @@ describe('Renderer', () => {
});
const RENDERED_CONTENTS =
`\n// ADD EXPORTS\n\n// REMOVE DECORATORS\n\n// ADD IMPORTS\n\n// ADD CONSTANTS\n\n// ADD DEFINITIONS\n` +
`\n// ADD EXPORTS\n\n// ADD IMPORTS\n\n// ADD CONSTANTS\n\n// ADD DEFINITIONS\n\n// REMOVE DECORATORS\n` +
INPUT_PROGRAM.contents;
const OUTPUT_PROGRAM_MAP = fromObject({