Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ngcc): render adjacent statements after static properties #33630

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions integration/ngcc/test.sh
Expand Up @@ -78,6 +78,10 @@ assertSucceeded "Expected 'ngcc' to log 'Compiling'."
grep "ApplicationModule.ɵmod = ɵngcc0.ɵɵdefineNgModule" node_modules/@angular/core/esm5/src/application_module.js
assertSucceeded "Expected 'ngcc' to correctly compile 'ApplicationModule' in '@angular/core' (esm5)."

# Did it place the `setClassMetadata` call correctly?
cat node_modules/@angular/core/fesm2015/core.js | awk 'ORS=" "' | grep "ApplicationRef.ctorParameters.*setClassMetadata(ApplicationRef"
assertSucceeded "Expected 'ngcc' to place 'setClassMetadata' after static properties like 'ctorParameters' in '@angular/core' (fesm2015)."


# Did it transform @angular/core typing files correctly?
grep "import [*] as ɵngcc0 from './src/r3_symbols';" node_modules/@angular/core/core.d.ts
Expand Down
Expand Up @@ -35,4 +35,25 @@ export class Esm5RenderingFormatter extends EsmRenderingFormatter {
const insertionPoint = returnStatement.getFullStart();
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);
}
}
Expand Up @@ -100,6 +100,30 @@ export class EsmRenderingFormatter implements RenderingFormatter {
output.appendLeft(insertionPoint, '\n' + definitions);
}

/**
* Add the adjacent statements after all static properties of the class.
*/
addAdjacentStatements(output: MagicString, compiledClass: CompiledClass, statements: string):
void {
const classSymbol = this.host.getClassSymbol(compiledClass.declaration);
if (!classSymbol) {
throw new Error(`Compiled class does not have a valid symbol: ${compiledClass.name}`);
}

let insertionPoint = classSymbol.declaration.valueDeclaration.getEnd();

// 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);
}

/**
* Remove static decorator properties from classes.
*/
Expand Down Expand Up @@ -244,3 +268,21 @@ function getNextSiblingInArray<T extends ts.Node>(node: T, array: ts.NodeArray<T
const index = array.indexOf(node);
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;
}
47 changes: 35 additions & 12 deletions packages/compiler-cli/ngcc/src/rendering/renderer.ts
Expand Up @@ -86,6 +86,10 @@ export class Renderer {
this.renderDefinitions(compiledFile.sourceFile, clazz, importManager);
this.srcFormatter.addDefinitions(outputText, clazz, renderedDefinition);

const renderedStatements =
this.renderAdjacentStatements(compiledFile.sourceFile, clazz, importManager);
this.srcFormatter.addAdjacentStatements(outputText, clazz, renderedStatements);

if (!isEntryPoint && clazz.reexports.length > 0) {
this.srcFormatter.addDirectExports(
outputText, clazz.reexports, importManager, compiledFile.sourceFile);
Expand Down Expand Up @@ -147,26 +151,45 @@ export class Renderer {
}

/**
* Render the definitions as source code for the given class.
* @param sourceFile The file containing the class to process.
* @param clazz The class whose definitions are to be rendered.
* @param compilation The results of analyzing the class - this is used to generate the rendered
* definitions.
* @param imports An object that tracks the imports that are needed by the rendered definitions.
*/
* Render the definitions as source code for the given class.
* @param sourceFile The file containing the class to process.
* @param clazz The class whose definitions are to be rendered.
* @param compilation The results of analyzing the class - this is used to generate the rendered
* definitions.
* @param imports An object that tracks the imports that are needed by the rendered definitions.
*/
private renderDefinitions(
sourceFile: ts.SourceFile, compiledClass: CompiledClass, imports: ImportManager): string {
const printer = createPrinter();
const name = this.host.getInternalNameOfClass(compiledClass.declaration);
const translate = (stmt: Statement) =>
translateStatement(stmt, imports, NOOP_DEFAULT_IMPORT_RECORDER);
const print = (stmt: Statement) =>
printer.printNode(ts.EmitHint.Unspecified, translate(stmt), sourceFile);
const statements: Statement[] = compiledClass.compilation.map(
c => { return createAssignmentStatement(name, c.name, c.initializer); });
return this.renderStatements(sourceFile, statements, imports);
}

/**
* Render the adjacent statements as source code for the given class.
* @param sourceFile The file containing the class to process.
* @param clazz The class whose statements are to be rendered.
* @param compilation The results of analyzing the class - this is used to generate the rendered
* definitions.
* @param imports An object that tracks the imports that are needed by the rendered definitions.
*/
private renderAdjacentStatements(
sourceFile: ts.SourceFile, compiledClass: CompiledClass, imports: ImportManager): string {
const statements: Statement[] = [];
for (const c of compiledClass.compilation) {
statements.push(...c.statements);
}
return this.renderStatements(sourceFile, statements, imports);
}

private renderStatements(
sourceFile: ts.SourceFile, statements: Statement[], imports: ImportManager): string {
const printer = createPrinter();
const translate = (stmt: Statement) =>
translateStatement(stmt, imports, NOOP_DEFAULT_IMPORT_RECORDER);
const print = (stmt: Statement) =>
printer.printNode(ts.EmitHint.Unspecified, translate(stmt), sourceFile);
return statements.map(print).join('\n');
}
}
Expand Down
Expand Up @@ -36,6 +36,8 @@ export interface RenderingFormatter {
output: MagicString, exports: Reexport[], importManager: ImportManager,
file: ts.SourceFile): void;
addDefinitions(output: MagicString, compiledClass: CompiledClass, definitions: string): void;
addAdjacentStatements(output: MagicString, compiledClass: CompiledClass, statements: string):
void;
removeDecorators(output: MagicString, decoratorsToRemove: RedundantDecoratorMap): void;
rewriteSwitchableDeclarations(
outputText: MagicString, sourceFile: ts.SourceFile,
Expand Down
Expand Up @@ -345,6 +345,55 @@ SOME DEFINITION TEXT
});
});

describe('addAdjacentStatements', () => {
const contents = `var core = require('@angular/core');\n` +
`var SomeDirective = /** @class **/ (function () {\n` +
` function SomeDirective(zone, cons) {}\n` +
` SomeDirective.prototype.method = function() {}\n` +
` SomeDirective.decorators = [\n` +
` { type: core.Directive, args: [{ selector: '[a]' }] },\n` +
` { type: OtherA }\n` +
` ];\n` +
` SomeDirective.ctorParameters = () => [\n` +
` { type: core.NgZone },\n` +
` { type: core.Console }\n` +
` ];\n` +
` return SomeDirective;\n` +
`}());\n` +
`export {SomeDirective};`;

it('should insert the statements after all the static methods of the class', () => {
const program = {name: _('/node_modules/test-package/some/file.js'), contents};
const {renderer, decorationAnalyses, sourceFile} = setup(program);
const output = new MagicString(contents);
const compiledClass = decorationAnalyses.get(sourceFile) !.compiledClasses.find(
c => c.name === 'SomeDirective') !;
renderer.addAdjacentStatements(output, compiledClass, 'SOME STATEMENTS');
expect(output.toString())
.toContain(
` SomeDirective.ctorParameters = () => [\n` +
` { type: core.NgZone },\n` +
` { type: core.Console }\n` +
` ];\n` +
`SOME STATEMENTS\n` +
` return SomeDirective;\n`);
});

it('should insert the statements after any definitions', () => {
const program = {name: _('/node_modules/test-package/some/file.js'), contents};
const {renderer, decorationAnalyses, sourceFile} = setup(program);
const output = new MagicString(contents);
const compiledClass = decorationAnalyses.get(sourceFile) !.compiledClasses.find(
c => c.name === 'SomeDirective') !;
renderer.addDefinitions(output, compiledClass, 'SOME DEFINITIONS');
renderer.addAdjacentStatements(output, compiledClass, 'SOME STATEMENTS');
const definitionsPosition = output.toString().indexOf('SOME DEFINITIONS');
const statementsPosition = output.toString().indexOf('SOME STATEMENTS');
expect(definitionsPosition).not.toEqual(-1, 'definitions should exist');
expect(statementsPosition).not.toEqual(-1, 'statements should exist');
expect(statementsPosition).toBeGreaterThan(definitionsPosition);
});
});

describe('removeDecorators', () => {

Expand Down
Expand Up @@ -39,6 +39,9 @@ class TestRenderingFormatter implements RenderingFormatter {
addDefinitions(output: MagicString, compiledClass: CompiledClass, definitions: string) {
output.prepend('\n// ADD DEFINITIONS\n');
}
addAdjacentStatements(output: MagicString, compiledClass: CompiledClass, statements: string) {
output.prepend('\n// ADD ADJACENT STATEMENTS\n');
}
removeDecorators(output: MagicString, decoratorsToRemove: RedundantDecoratorMap) {
output.prepend('\n// REMOVE DECORATORS\n');
}
Expand Down Expand Up @@ -79,6 +82,7 @@ function createTestRenderer(
spyOn(testFormatter, 'addExports').and.callThrough();
spyOn(testFormatter, 'addImports').and.callThrough();
spyOn(testFormatter, 'addDefinitions').and.callThrough();
spyOn(testFormatter, 'addAdjacentStatements').and.callThrough();
spyOn(testFormatter, 'addConstants').and.callThrough();
spyOn(testFormatter, 'removeDecorators').and.callThrough();
spyOn(testFormatter, 'rewriteSwitchableDeclarations').and.callThrough();
Expand Down
Expand Up @@ -334,6 +334,56 @@ SOME DEFINITION TEXT
});
});

describe('addAdjacentStatements', () => {
const contents = `import {Directive, NgZone, Console} from '@angular/core';\n` +
`var SomeDirective = /** @class **/ (function () {\n` +
` function SomeDirective(zone, cons) {}\n` +
` SomeDirective.prototype.method = function() {}\n` +
` SomeDirective.decorators = [\n` +
` { type: Directive, args: [{ selector: '[a]' }] },\n` +
` { type: OtherA }\n` +
` ];\n` +
` SomeDirective.ctorParameters = () => [\n` +
` { type: NgZone },\n` +
` { type: Console }\n` +
` ];\n` +
` return SomeDirective;\n` +
`}());\n` +
`export {SomeDirective};`;

it('should insert the statements after all the static methods of the class', () => {
const program = {name: _('/node_modules/test-package/some/file.js'), contents};
const {renderer, decorationAnalyses, sourceFile} = setup(program);
const output = new MagicString(contents);
const compiledClass = decorationAnalyses.get(sourceFile) !.compiledClasses.find(
c => c.name === 'SomeDirective') !;
renderer.addAdjacentStatements(output, compiledClass, 'SOME STATEMENTS');
expect(output.toString())
.toContain(
` SomeDirective.ctorParameters = () => [\n` +
` { type: NgZone },\n` +
` { type: Console }\n` +
` ];\n` +
`SOME STATEMENTS\n` +
` return SomeDirective;\n`);
});

it('should insert the statements after any definitions', () => {
const program = {name: _('/node_modules/test-package/some/file.js'), contents};
const {renderer, decorationAnalyses, sourceFile} = setup(program);
const output = new MagicString(contents);
const compiledClass = decorationAnalyses.get(sourceFile) !.compiledClasses.find(
c => c.name === 'SomeDirective') !;
renderer.addDefinitions(output, compiledClass, 'SOME DEFINITIONS');
renderer.addAdjacentStatements(output, compiledClass, 'SOME STATEMENTS');
const definitionsPosition = output.toString().indexOf('SOME DEFINITIONS');
const statementsPosition = output.toString().indexOf('SOME STATEMENTS');
expect(definitionsPosition).not.toEqual(-1, 'definitions should exist');
expect(statementsPosition).not.toEqual(-1, 'statements should exist');
expect(statementsPosition).toBeGreaterThan(definitionsPosition);
});
});


describe('removeDecorators', () => {

Expand Down
Expand Up @@ -241,9 +241,54 @@ SOME DEFINITION TEXT
A.decorators = [
`);
});

});

describe('addAdjacentStatements', () => {
const 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` +
`];`;

it('should insert the statements after all the static methods of the class', () => {
const program = {name: _('/node_modules/test-package/some/file.js'), contents};
const {renderer, decorationAnalyses, sourceFile} = setup([program]);
const output = new MagicString(contents);
const compiledClass = decorationAnalyses.get(sourceFile) !.compiledClasses.find(
c => c.name === 'SomeDirective') !;
renderer.addAdjacentStatements(output, compiledClass, 'SOME STATEMENTS');
expect(output.toString())
.toContain(
`SomeDirective.ctorParameters = () => [\n` +
` { type: NgZone },\n` +
` { type: Console }\n` +
`];\n` +
`SOME STATEMENTS`);
});

it('should insert the statements after any definitions', () => {
const program = {name: _('/node_modules/test-package/some/file.js'), contents};
const {renderer, decorationAnalyses, sourceFile} = setup([program]);
const output = new MagicString(contents);
const compiledClass = decorationAnalyses.get(sourceFile) !.compiledClasses.find(
c => c.name === 'SomeDirective') !;
renderer.addDefinitions(output, compiledClass, 'SOME DEFINITIONS');
renderer.addAdjacentStatements(output, compiledClass, 'SOME STATEMENTS');
const definitionsPosition = output.toString().indexOf('SOME DEFINITIONS');
const statementsPosition = output.toString().indexOf('SOME STATEMENTS');
expect(definitionsPosition).not.toEqual(-1, 'definitions should exist');
expect(statementsPosition).not.toEqual(-1, 'statements should exist');
expect(statementsPosition).toBeGreaterThan(definitionsPosition);
});
});

describe('removeDecorators', () => {
describe('[static property declaration]', () => {
Expand Down