fix(ivy): ngcc - insert new imports after existing ones (#30029)

Previously, ngcc would insert new imports at the beginning of the file, for
convenience. This is problematic for imports that have side-effects, as the
side-effects imposed by such imports may affect the behavior of subsequent
imports.

This commit teaches ngcc to insert imports after any existing imports. Special
care has been taken to ensure inserted constants will still follow after the
inserted imports.

Resolves FW-1271

PR Close #30029

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

@@ -26,10 +26,13 @@ export class EsmRenderer extends Renderer {
   /**
    *  Add the imports at the top of the file
    */
-  addImports(output: MagicString, imports: {specifier: string; qualifier: string;}[]): void {
-    // The imports get inserted at the very top of the file.
-    imports.forEach(
-        i => { output.appendLeft(0, `import * as ${i.qualifier} from '${i.specifier}';\n`); });
+  addImports(
+      output: MagicString, imports: {specifier: string; qualifier: string;}[],
+      sf: ts.SourceFile): void {
+    const insertionPoint = findEndOfImports(sf);
+    const renderedImports =
+        imports.map(i => `import * as ${i.qualifier} from '${i.specifier}';\n`).join('');
+    output.appendLeft(insertionPoint, renderedImports);
   }
 
   addExports(output: MagicString, entryPointBasePath: string, exports: ExportInfo[]): void {
@@ -55,14 +58,11 @@ export class EsmRenderer extends Renderer {
     if (constants === '') {
       return;
     }
-    const insertionPoint = file.statements.reduce((prev, stmt) => {
-      if (ts.isImportDeclaration(stmt) || ts.isImportEqualsDeclaration(stmt) ||
-          ts.isNamespaceImport(stmt)) {
-        return stmt.getEnd();
-      }
-      return prev;
-    }, 0);
-    output.appendLeft(insertionPoint, '\n' + constants + '\n');
+    const insertionPoint = findEndOfImports(file);
+
+    // Append the constants to the right of the insertion point, to ensure they get ordered after
+    // added imports (those are appended left to the insertion point).
+    output.appendRight(insertionPoint, '\n' + constants + '\n');
   }
 
   /**
@@ -115,6 +115,17 @@ export class EsmRenderer extends Renderer {
   }
 }
 
+function findEndOfImports(sf: ts.SourceFile): number {
+  for (const stmt of sf.statements) {
+    if (!ts.isImportDeclaration(stmt) && !ts.isImportEqualsDeclaration(stmt) &&
+        !ts.isNamespaceImport(stmt)) {
+      return stmt.getStart();
+    }
+  }
+
+  return 0;
+}
+
 function findStatement(node: ts.Node) {
   while (node) {
     if (ts.isExpressionStatement(node)) {

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

@@ -155,7 +155,9 @@ export abstract class Renderer {
           renderConstantPool(compiledFile.sourceFile, compiledFile.constantPool, importManager),
           compiledFile.sourceFile);
 
-      this.addImports(outputText, importManager.getAllImports(compiledFile.sourceFile.fileName));
+      this.addImports(
+          outputText, importManager.getAllImports(compiledFile.sourceFile.fileName),
+          compiledFile.sourceFile);
     }
 
     // Add exports to the entry-point file
@@ -185,7 +187,7 @@ export abstract class Renderer {
     });
 
     this.addModuleWithProvidersParams(outputText, renderInfo.moduleWithProviders, importManager);
-    this.addImports(outputText, importManager.getAllImports(dtsFile.fileName));
+    this.addImports(outputText, importManager.getAllImports(dtsFile.fileName), dtsFile);
 
     this.addExports(outputText, dtsFile.fileName, renderInfo.privateExports);
 
@@ -246,8 +248,9 @@ export abstract class Renderer {
 
   protected abstract addConstants(output: MagicString, constants: string, file: ts.SourceFile):
       void;
-  protected abstract addImports(output: MagicString, imports: {specifier: string,
-                                                               qualifier: string}[]): void;
+  protected abstract addImports(
+      output: MagicString, imports: {specifier: string, qualifier: string}[],
+      sf: ts.SourceFile): void;
   protected abstract addExports(
       output: MagicString, entryPointBasePath: string, exports: ExportInfo[]): void;
   protected abstract addDefinitions(

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

@@ -42,6 +42,7 @@ const PROGRAM = {
   name: '/some/file.js',
   contents: `
 /* A copyright notice */
+import 'some-side-effect';
 import {Directive} from '@angular/core';
 export class A {}
 A.decorators = [
@@ -114,17 +115,21 @@ export { D };
 describe('Esm2015Renderer', () => {
 
   describe('addImports', () => {
-    it('should insert the given imports at the start of the source file', () => {
-      const {renderer} = setup(PROGRAM);
+    it('should insert the given imports after existing imports of the source file', () => {
+      const {renderer, sourceFile} = setup(PROGRAM);
       const output = new MagicString(PROGRAM.contents);
-      renderer.addImports(output, [
+      renderer.addImports(
+          output,
+          [
             {specifier: '@angular/core', qualifier: 'i0'},
             {specifier: '@angular/common', qualifier: 'i1'}
-      ]);
-      expect(output.toString()).toContain(`import * as i0 from '@angular/core';
-import * as i1 from '@angular/common';
-
-/* A copyright notice */`);
+          ],
+          sourceFile);
+      expect(output.toString()).toContain(`/* A copyright notice */
+import 'some-side-effect';
+import {Directive} from '@angular/core';
+import * as i0 from '@angular/core';
+import * as i1 from '@angular/common';`);
     });
   });
 
@@ -173,10 +178,27 @@ export {TopLevelComponent};`);
       renderer.addConstants(output, 'const x = 3;', file);
       expect(output.toString()).toContain(`
 import {Directive} from '@angular/core';
-const x = 3;
 
+const x = 3;
 export class A {}`);
     });
+
+    it('should insert constants after inserted imports', () => {
+      const {renderer, program} = setup(PROGRAM);
+      const file = program.getSourceFile('some/file.js');
+      if (file === undefined) {
+        throw new Error(`Could not find source file`);
+      }
+      const output = new MagicString(PROGRAM.contents);
+      renderer.addConstants(output, 'const x = 3;', file);
+      renderer.addImports(output, [{specifier: '@angular/core', qualifier: 'i0'}], file);
+      expect(output.toString()).toContain(`
+import {Directive} from '@angular/core';
+import * as i0 from '@angular/core';
+
+const x = 3;
+export class A {`);
+    });
   });
 
   describe('rewriteSwitchableDeclarations', () => {

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

@@ -42,6 +42,7 @@ const PROGRAM = {
   name: '/some/file.js',
   contents: `
 /* A copyright notice */
+import 'some-side-effect';
 import {Directive} from '@angular/core';
 var A = (function() {
   function A() {}
@@ -151,17 +152,21 @@ export { D };
 describe('Esm5Renderer', () => {
 
   describe('addImports', () => {
-    it('should insert the given imports at the start of the source file', () => {
-      const {renderer} = setup(PROGRAM);
+    it('should insert the given imports after existing imports of the source file', () => {
+      const {renderer, sourceFile} = setup(PROGRAM);
       const output = new MagicString(PROGRAM.contents);
-      renderer.addImports(output, [
+      renderer.addImports(
+          output,
+          [
             {specifier: '@angular/core', qualifier: 'i0'},
             {specifier: '@angular/common', qualifier: 'i1'}
-      ]);
-      expect(output.toString()).toContain(`import * as i0 from '@angular/core';
-import * as i1 from '@angular/common';
-
-/* A copyright notice */`);
+          ],
+          sourceFile);
+      expect(output.toString()).toContain(`/* A copyright notice */
+import 'some-side-effect';
+import {Directive} from '@angular/core';
+import * as i0 from '@angular/core';
+import * as i1 from '@angular/common';`);
     });
   });
 
@@ -210,8 +215,25 @@ export {TopLevelComponent};`);
       renderer.addConstants(output, 'const x = 3;', file);
       expect(output.toString()).toContain(`
 import {Directive} from '@angular/core';
-const x = 3;
 
+const x = 3;
+var A = (function() {`);
+    });
+
+    it('should insert constants after inserted imports', () => {
+      const {renderer, program} = setup(PROGRAM);
+      const file = program.getSourceFile('some/file.js');
+      if (file === undefined) {
+        throw new Error(`Could not find source file`);
+      }
+      const output = new MagicString(PROGRAM.contents);
+      renderer.addConstants(output, 'const x = 3;', file);
+      renderer.addImports(output, [{specifier: '@angular/core', qualifier: 'i0'}], file);
+      expect(output.toString()).toContain(`
+import {Directive} from '@angular/core';
+import * as i0 from '@angular/core';
+
+const x = 3;
 var A = (function() {`);
     });
   });

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

@@ -26,7 +26,8 @@ class TestRenderer extends Renderer {
       logger: Logger, host: Esm2015ReflectionHost, isCore: boolean, bundle: EntryPointBundle) {
     super(logger, host, isCore, bundle, '/src');
   }
-  addImports(output: MagicString, imports: {specifier: string, qualifier: string}[]) {
+  addImports(
+      output: MagicString, imports: {specifier: string, qualifier: string}[], sf: ts.SourceFile) {
     output.prepend('\n// ADD IMPORTS\n');
   }
   addExports(output: MagicString, baseEntryPointPath: string, exports: {
@@ -510,11 +511,15 @@ describe('Renderer', () => {
             export declare function withProviders7(): ({ngModule: SomeModule, providers: any[]})&{ngModule:SomeModule};
             export declare function withProviders8(): (MyModuleWithProviders)&{ngModule:SomeModule};`);
 
-        expect(renderer.addImports).toHaveBeenCalledWith(jasmine.any(MagicString), [
+        expect(renderer.addImports)
+            .toHaveBeenCalledWith(
+                jasmine.any(MagicString),
+                [
                   {specifier: './module', qualifier: 'ɵngcc0'},
                   {specifier: '@angular/core', qualifier: 'ɵngcc1'},
                   {specifier: 'some-library', qualifier: 'ɵngcc2'},
-        ]);
+                ],
+                jasmine.anything());
 
 
         // The following expectation checks that we do not mistake `ModuleWithProviders` types