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

See angular#33337 (comment) Fixes FW-1664 PR Close angular#33630

Author: petebacondarwin

integration/ngcc/test.sh

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

packages/compiler-cli/ngcc/src/rendering/esm5_rendering_formatter.ts

@@ -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);
+ }
 }

packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts

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

packages/compiler-cli/ngcc/src/rendering/renderer.ts

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

packages/compiler-cli/ngcc/src/rendering/rendering_formatter.ts

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

packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts

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

packages/compiler-cli/ngcc/test/rendering/dts_renderer_spec.ts

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

packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts

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

packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts

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