fix(ngcc): ensure that adjacent statements go after helper calls (#33689)

Previously the renderers were fixed so that they inserted extra
"adjacent" statements after the last static property of classes.

In order to help the build-optimizer (in Angular CLI) to be able to
tree-shake classes effectively, these statements should also appear
after any helper calls, such as `__decorate()`.

This commit moves the computation of this positioning into the
`NgccReflectionHost` via the `getEndOfClass()` method, which
returns the last statement that is related to the class.

FW-1668

PR Close #33689
This commit is contained in:
Pete Bacon Darwin 2019-11-08 11:01:26 +00:00 committed by Kara Erickson
parent 52d1500155
commit b3c3000004
9 changed files with 172 additions and 52 deletions

View File

@ -534,6 +534,35 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
return infos; return infos;
} }
getEndOfClass(classSymbol: NgccClassSymbol): ts.Node {
let last: ts.Node = classSymbol.declaration.valueDeclaration;
// If there are static members on this class then find the last one
if (classSymbol.declaration.exports !== undefined) {
classSymbol.declaration.exports.forEach(exportSymbol => {
if (exportSymbol.valueDeclaration === undefined) {
return;
}
const exportStatement = getContainingStatement(exportSymbol.valueDeclaration);
if (exportStatement !== null && last.getEnd() < exportStatement.getEnd()) {
last = exportStatement;
}
});
}
// If there are helper calls for this class then find the last one
const helpers = this.getHelperCallsForClass(
classSymbol, ['__decorate', '__extends', '__param', '__metadata']);
helpers.forEach(helper => {
const helperStatement = getContainingStatement(helper);
if (helperStatement !== null && last.getEnd() < helperStatement.getEnd()) {
last = helperStatement;
}
});
return last;
}
///////////// Protected Helpers ///////////// ///////////// Protected Helpers /////////////
/** /**
@ -1891,3 +1920,17 @@ function isSynthesizedSuperCall(expression: ts.Expression): boolean {
return ts.isSpreadElement(argument) && ts.isIdentifier(argument.expression) && return ts.isSpreadElement(argument) && ts.isIdentifier(argument.expression) &&
argument.expression.text === 'arguments'; argument.expression.text === 'arguments';
} }
/**
* Find the statement that contains the given node
* @param node a node whose containing statement we wish to find
*/
function getContainingStatement(node: ts.Node): ts.ExpressionStatement|null {
while (node) {
if (ts.isExpressionStatement(node)) {
break;
}
node = node.parent;
}
return node || null;
}

View File

@ -103,6 +103,22 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost {
return this.getInternalNameOfClass(clazz); return this.getInternalNameOfClass(clazz);
} }
getEndOfClass(classSymbol: NgccClassSymbol): ts.Node {
const iifeBody = getIifeBody(classSymbol.declaration.valueDeclaration);
if (!iifeBody) {
throw new Error(
`Compiled class declaration is not inside an IIFE: ${classSymbol.name} in ${classSymbol.declaration.valueDeclaration.getSourceFile().fileName}`);
}
const returnStatementIndex = iifeBody.statements.findIndex(ts.isReturnStatement);
if (returnStatementIndex === -1) {
throw new Error(
`Compiled class wrapper IIFE does not have a return statement: ${classSymbol.name} in ${classSymbol.declaration.valueDeclaration.getSourceFile().fileName}`);
}
// Return the statement before the IIFE return statement
return iifeBody.statements[returnStatementIndex - 1];
}
/** /**
* In ES5, the implementation of a class is a function expression that is hidden inside an IIFE, * In ES5, the implementation of a class is a function expression that is hidden inside an IIFE,
* whose value is assigned to a variable (which represents the class to the rest of the program). * whose value is assigned to a variable (which represents the class to the rest of the program).

View File

@ -116,4 +116,16 @@ export interface NgccReflectionHost extends ReflectionHost {
* objects. * objects.
*/ */
getModuleWithProvidersFunctions(f: ts.SourceFile): ModuleWithProvidersFunction[]; getModuleWithProvidersFunctions(f: ts.SourceFile): ModuleWithProvidersFunction[];
/**
* Find the last node that is relevant to the specified class.
*
* As well as the main declaration, classes can have additional statements such as static
* properties (`SomeClass.staticProp = ...;`) and decorators (`__decorate(SomeClass, ...);`).
* It is useful to know exactly where the class "ends" so that we can inject additional
* statements after that point.
*
* @param classSymbol The class whose statements we want.
*/
getEndOfClass(classSymbol: NgccClassSymbol): ts.Node;
} }

View File

@ -35,25 +35,4 @@ export class Esm5RenderingFormatter extends EsmRenderingFormatter {
const insertionPoint = returnStatement.getFullStart(); const insertionPoint = returnStatement.getFullStart();
output.appendLeft(insertionPoint, '\n' + definitions); output.appendLeft(insertionPoint, '\n' + definitions);
} }
/**
* Add the adjacent statements inside the IIFE of each decorated class
*/
addAdjacentStatements(output: MagicString, compiledClass: CompiledClass, statements: string):
void {
const iifeBody = getIifeBody(compiledClass.declaration);
if (!iifeBody) {
throw new Error(
`Compiled class declaration is not inside an IIFE: ${compiledClass.name} in ${compiledClass.declaration.getSourceFile().fileName}`);
}
const returnStatement = iifeBody.statements.find(ts.isReturnStatement);
if (!returnStatement) {
throw new Error(
`Compiled class wrapper IIFE does not have a return statement: ${compiledClass.name} in ${compiledClass.declaration.getSourceFile().fileName}`);
}
const insertionPoint = returnStatement.getFullStart();
output.appendLeft(insertionPoint, '\n' + statements);
}
} }

View File

@ -109,19 +109,8 @@ export class EsmRenderingFormatter implements RenderingFormatter {
if (!classSymbol) { if (!classSymbol) {
throw new Error(`Compiled class does not have a valid symbol: ${compiledClass.name}`); throw new Error(`Compiled class does not have a valid symbol: ${compiledClass.name}`);
} }
const endOfClass = this.host.getEndOfClass(classSymbol);
let insertionPoint = classSymbol.declaration.valueDeclaration.getEnd(); output.appendLeft(endOfClass.getEnd(), '\n' + statements);
// If there are static members on this class then insert after the last one
if (classSymbol.declaration.exports !== undefined) {
classSymbol.declaration.exports.forEach(exportSymbol => {
const exportStatement = getContainingStatement(exportSymbol);
if (exportStatement !== null) {
insertionPoint = Math.max(insertionPoint, exportStatement.getEnd());
}
});
}
output.appendLeft(insertionPoint, '\n' + statements);
} }
/** /**
@ -268,21 +257,3 @@ function getNextSiblingInArray<T extends ts.Node>(node: T, array: ts.NodeArray<T
const index = array.indexOf(node); const index = array.indexOf(node);
return index !== -1 && array.length > index + 1 ? array[index + 1] : null; return index !== -1 && array.length > index + 1 ? array[index + 1] : null;
} }
/**
* Find the statement that contains the given class member
* @param symbol the symbol of a static member of a class
*/
function getContainingStatement(symbol: ts.Symbol): ts.ExpressionStatement|null {
if (symbol.valueDeclaration === undefined) {
return null;
}
let node: ts.Node|null = symbol.valueDeclaration;
while (node) {
if (ts.isExpressionStatement(node)) {
break;
}
node = node.parent;
}
return node || null;
}

View File

@ -486,6 +486,20 @@ runInEachFileSystem(() => {
expect(value).toBe(null); expect(value).toBe(null);
}); });
}); });
describe('getEndOfClass()', () => {
it('should return the last statement related to the class', () => {
const {program} = makeTestBundleProgram(_('/ngmodule.js'));
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker());
const classSymbol =
host.findClassSymbols(program.getSourceFile(_('/ngmodule.js')) !)[0];
const endOfClass = host.getEndOfClass(classSymbol);
expect(endOfClass.getText())
.toMatch(
/HttpClientXsrfModule = HttpClientXsrfModule_1 = .*__decorate.*\(\[\n\s*NgModule\(\{\n\s*providers: \[],\n\s*}\)\n\s*], HttpClientXsrfModule\);/);
});
});
}); });
}); });

View File

@ -2160,5 +2160,62 @@ runInEachFileSystem(() => {
]); ]);
}); });
}); });
describe('getEndOfClass()', () => {
it('should return the last static property of the class', () => {
const testFile: TestFile = {
name: _('/node_modules/test-package/some/file.js'),
contents: `import {Directive, NgZone, Console} from '@angular/core';\n` +
`export class SomeDirective {\n` +
` constructor(zone, cons) {}\n` +
` method() {}\n` +
`}\n` +
`SomeDirective.decorators = [\n` +
` { type: Directive, args: [{ selector: '[a]' }] },\n` +
` { type: OtherA }\n` +
`];\n` +
`SomeDirective.ctorParameters = () => [\n` +
` { type: NgZone },\n` +
` { type: Console }\n` +
`];\n` +
`callSomeFunction();\n` +
`var value = 100;\n`
};
loadTestFiles([testFile]);
const {program} = makeTestBundleProgram(testFile.name);
const host = new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker());
const classSymbol = host.findClassSymbols(program.getSourceFile(testFile.name) !)[0];
const endOfClass = host.getEndOfClass(classSymbol);
expect(endOfClass.getText())
.toEqual(
`SomeDirective.ctorParameters = () => [\n` +
` { type: NgZone },\n` +
` { type: Console }\n` +
`];`);
});
it('should return the class declaration if there are no extra statements', () => {
const testFile: TestFile = {
name: _('/node_modules/test-package/some/file.js'),
contents: `export class SomeDirective {\n` +
` constructor(zone, cons) {}\n` +
` method() {}\n` +
`}\n` +
`callSomeFunction();\n` +
`var value = 100;\n`
};
loadTestFiles([testFile]);
const {program} = makeTestBundleProgram(testFile.name);
const host = new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker());
const classSymbol = host.findClassSymbols(program.getSourceFile(testFile.name) !)[0];
const endOfClass = host.getEndOfClass(classSymbol);
expect(endOfClass.getText())
.toEqual(
`export class SomeDirective {\n` +
` constructor(zone, cons) {}\n` +
` method() {}\n` +
`}`);
});
});
}); });
}); });

View File

@ -532,6 +532,19 @@ export { SomeDirective };
expect(value).toBe(null); expect(value).toBe(null);
}); });
}); });
describe('getEndOfClass()', () => {
it('should return the last statement related to the class', () => {
const {program} = makeTestBundleProgram(_('/ngmodule.js'));
const host = new Esm5ReflectionHost(new MockLogger(), false, program.getTypeChecker());
const classSymbol =
host.findClassSymbols(program.getSourceFile(_('/ngmodule.js')) !)[0];
const endOfClass = host.getEndOfClass(classSymbol);
expect(endOfClass.getText())
.toMatch(
/HttpClientXsrfModule = HttpClientXsrfModule_1 = .*__decorate.*\(\[\n\s*NgModule\(\{\n\s*providers: \[],\n\s*}\)\n\s*], HttpClientXsrfModule\);/);
});
});
}); });
function findVariableDeclaration( function findVariableDeclaration(

View File

@ -2530,5 +2530,20 @@ runInEachFileSystem(() => {
]); ]);
}); });
}); });
describe('getEndOfClass()', () => {
it('should return the last static property of the class', () => {
loadTestFiles([SOME_DIRECTIVE_FILE]);
const {program} = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name);
const host = new Esm5ReflectionHost(new MockLogger(), false, program.getTypeChecker());
const classSymbol =
host.findClassSymbols(program.getSourceFile(SOME_DIRECTIVE_FILE.name) !)[0];
const endOfClass = host.getEndOfClass(classSymbol);
expect(endOfClass.getText()).toEqual(`SomeDirective.propDecorators = {
"input1": [{ type: Input },],
"input2": [{ type: Input },],
};`);
});
});
}); });
}); });