From 8c80b851c868a7440be7e6782e8c9c9ab00f6e32 Mon Sep 17 00:00:00 2001 From: JoostK Date: Mon, 22 Apr 2019 14:57:31 +0200 Subject: [PATCH] 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 --- .../ngcc/src/rendering/esm_renderer.ts | 35 ++++++++++----- .../ngcc/src/rendering/renderer.ts | 11 +++-- .../test/rendering/esm2015_renderer_spec.ts | 44 ++++++++++++++----- .../ngcc/test/rendering/esm5_renderer_spec.ts | 44 ++++++++++++++----- .../ngcc/test/rendering/renderer_spec.ts | 17 ++++--- 5 files changed, 107 insertions(+), 44 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/rendering/esm_renderer.ts b/packages/compiler-cli/ngcc/src/rendering/esm_renderer.ts index 8de2a47f46..8e6b4f2f10 100644 --- a/packages/compiler-cli/ngcc/src/rendering/esm_renderer.ts +++ b/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)) { diff --git a/packages/compiler-cli/ngcc/src/rendering/renderer.ts b/packages/compiler-cli/ngcc/src/rendering/renderer.ts index 172a2abe55..39d0a9caa1 100644 --- a/packages/compiler-cli/ngcc/src/rendering/renderer.ts +++ b/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( diff --git a/packages/compiler-cli/ngcc/test/rendering/esm2015_renderer_spec.ts b/packages/compiler-cli/ngcc/test/rendering/esm2015_renderer_spec.ts index 894d0c6540..d36d5838f6 100644 --- a/packages/compiler-cli/ngcc/test/rendering/esm2015_renderer_spec.ts +++ b/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, [ - {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 */`); + renderer.addImports( + output, + [ + {specifier: '@angular/core', qualifier: 'i0'}, + {specifier: '@angular/common', qualifier: 'i1'} + ], + 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', () => { diff --git a/packages/compiler-cli/ngcc/test/rendering/esm5_renderer_spec.ts b/packages/compiler-cli/ngcc/test/rendering/esm5_renderer_spec.ts index 03d987862f..a7d7c17614 100644 --- a/packages/compiler-cli/ngcc/test/rendering/esm5_renderer_spec.ts +++ b/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, [ - {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 */`); + renderer.addImports( + output, + [ + {specifier: '@angular/core', qualifier: 'i0'}, + {specifier: '@angular/common', qualifier: 'i1'} + ], + 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() {`); }); }); diff --git a/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts b/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts index 177b342c86..cea122f6af 100644 --- a/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts +++ b/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), [ - {specifier: './module', qualifier: 'ɵngcc0'}, - {specifier: '@angular/core', qualifier: 'ɵngcc1'}, - {specifier: 'some-library', qualifier: 'ɵngcc2'}, - ]); + 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