From 10e29355db1864ba89cf5695dfc9c1e4017afc98 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Mon, 23 Dec 2019 16:07:34 +0200 Subject: [PATCH] fix(ngcc): do not add trailing commas in UMD imports (#34545) Previously, if `UmdRenderingFormatter#addImports()` was called with an empty list of imports to add (i.e. no new imports were needed), it would add trailing commas in several locations (arrays, function arguments, function parameters), thus making the code imcompatible with legacy browsers such as IE11. This commit fixes it by ensuring that no trailing commas are added if `addImports()` is called with an empty list of imports. This is a follow-up to #34353. Fixes #34525 PR Close #34545 --- integration/ngcc/test.sh | 15 +++++++++++++++ .../src/rendering/commonjs_rendering_formatter.ts | 5 +++++ .../ngcc/src/rendering/esm_rendering_formatter.ts | 6 +++++- .../ngcc/src/rendering/umd_rendering_formatter.ts | 4 ++++ .../commonjs_rendering_formatter_spec.ts | 11 +++++++++++ .../rendering/esm5_rendering_formatter_spec.ts | 11 +++++++++++ .../rendering/esm_rendering_formatter_spec.ts | 11 +++++++++++ .../rendering/umd_rendering_formatter_spec.ts | 12 ++++++++++++ 8 files changed, 74 insertions(+), 1 deletion(-) diff --git a/integration/ngcc/test.sh b/integration/ngcc/test.sh index b545def610..e17972a122 100755 --- a/integration/ngcc/test.sh +++ b/integration/ngcc/test.sh @@ -125,6 +125,21 @@ assertSucceeded "Expected 'ngcc' to log 'Compiling'." assertSucceeded "Expected '@angular/cdk/a11y' (umd) to actually have decorators via static properties." +# Did it transform imports in UMD correctly? +# (E.g. no trailing commas, so that it remains compatible with legacy browsers, such as IE11.) + grep "factory(exports, require('rxjs'), require('rxjs/operators'))" node_modules/@angular/core/bundles/core.umd.js + assertSucceeded "Expected 'ngcc' to not add trailing commas to CommonJS block in UMD." + + grep "define('@angular/core', \['exports', 'rxjs', 'rxjs/operators'], factory)" node_modules/@angular/core/bundles/core.umd.js + assertSucceeded "Expected 'ngcc' to not add trailing commas to AMD block in UMD." + + grep "factory((global.ng = global.ng || {}, global.ng.core = {}), global.rxjs, global.rxjs.operators)" node_modules/@angular/core/bundles/core.umd.js + assertSucceeded "Expected 'ngcc' to not add trailing commas to globals block in UMD." + + grep "(this, (function (exports, rxjs, operators) {" node_modules/@angular/core/bundles/core.umd.js + assertSucceeded "Expected 'ngcc' to not add trailing commas to factory function parameters in UMD." + + # Can it be safely run again (as a noop)? # And check that it logged skipping compilation as expected ngcc -l debug | grep 'Skipping' diff --git a/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts b/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts index 65200cb991..30938fc5ac 100644 --- a/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts +++ b/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts @@ -30,6 +30,11 @@ export class CommonJsRenderingFormatter extends Esm5RenderingFormatter { * Add the imports below any in situ imports as `require` calls. */ addImports(output: MagicString, imports: Import[], file: ts.SourceFile): void { + // Avoid unnecessary work if there are no imports to add. + if (imports.length === 0) { + return; + } + const insertionPoint = this.findEndOfImports(file); const renderedImports = imports.map(i => `var ${i.qualifier} = require('${i.specifier}');\n`).join(''); diff --git a/packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts b/packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts index 54e46f63fb..98a35134e2 100644 --- a/packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts +++ b/packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts @@ -32,6 +32,10 @@ export class EsmRenderingFormatter implements RenderingFormatter { * Add the imports at the top of the file, after any imports that are already there. */ addImports(output: MagicString, imports: Import[], sf: ts.SourceFile): void { + if (imports.length === 0) { + return; + } + const insertionPoint = this.findEndOfImports(sf); const renderedImports = imports.map(i => `import * as ${i.qualifier} from '${i.specifier}';\n`).join(''); @@ -294,4 +298,4 @@ function getEndExceptSemicolon(statement: ts.Statement): number { const lastToken = statement.getLastToken(); return (lastToken && lastToken.kind === ts.SyntaxKind.SemicolonToken) ? statement.getEnd() - 1 : statement.getEnd(); -} \ No newline at end of file +} diff --git a/packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts b/packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts index 0b3dc215c8..2319171bce 100644 --- a/packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts +++ b/packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts @@ -30,6 +30,10 @@ export class UmdRenderingFormatter extends Esm5RenderingFormatter { * Add the imports to the UMD module IIFE. */ addImports(output: MagicString, imports: Import[], file: ts.SourceFile): void { + if (imports.length === 0) { + return; + } + // Assume there is only one UMD module in the file const umdModule = this.umdHost.getUmdModule(file); if (!umdModule) { diff --git a/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts index 30a1390a6c..ad96ba569b 100644 --- a/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts @@ -187,6 +187,17 @@ var core = require('@angular/core'); var i0 = require('@angular/core'); var i1 = require('@angular/common');`); }); + + it('should leave the file unchanged if there are no imports to add', () => { + const {renderer, sourceFile} = setup(PROGRAM); + const output = new MagicString(PROGRAM.contents); + const contentsBefore = output.toString(); + + renderer.addImports(output, [], sourceFile); + const contentsAfter = output.toString(); + + expect(contentsAfter).toBe(contentsBefore); + }); }); describe('addExports', () => { diff --git a/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts index 79701f3378..d98f4d73cc 100644 --- a/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts @@ -195,6 +195,17 @@ import {Directive} from '@angular/core'; import * as i0 from '@angular/core'; import * as i1 from '@angular/common';`); }); + + it('should leave the file unchanged if there are no imports to add', () => { + const {renderer, sourceFile} = setup(PROGRAM); + const output = new MagicString(PROGRAM.contents); + const contentsBefore = output.toString(); + + renderer.addImports(output, [], sourceFile); + const contentsAfter = output.toString(); + + expect(contentsAfter).toBe(contentsBefore); + }); }); describe('addExports', () => { diff --git a/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts index 2336cd742e..85d43e230f 100644 --- a/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts @@ -112,6 +112,17 @@ import {Directive} from '@angular/core'; import * as i0 from '@angular/core'; import * as i1 from '@angular/common';`); }); + + it('should leave the file unchanged if there are no imports to add', () => { + const {renderer, sourceFile} = setup([PROGRAM]); + const output = new MagicString(PROGRAM.contents); + const contentsBefore = output.toString(); + + renderer.addImports(output, [], sourceFile); + const contentsAfter = output.toString(); + + expect(contentsAfter).toBe(contentsBefore); + }); }); describe('addExports', () => { diff --git a/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts index 0a0a8bc665..07f3e11420 100644 --- a/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts @@ -325,6 +325,18 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', expect(outputSrc).toContain(`(factory(global.ng.core,global.ng.common));`); expect(outputSrc).toContain(`(function (i0,i1) {'use strict';`); }); + + it('should leave the file unchanged if there are no imports to add', () => { + const {renderer, program} = setup(PROGRAM); + const file = getSourceFileOrError(program, _('/node_modules/test-package/some/file.js')); + const output = new MagicString(PROGRAM.contents); + const contentsBefore = output.toString(); + + renderer.addImports(output, [], file); + const contentsAfter = output.toString(); + + expect(contentsAfter).toBe(contentsBefore); + }); }); describe('addExports', () => {