Skip to content
Permalink
Browse files

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
  • Loading branch information
gkalpak authored and alxhub committed Dec 23, 2019
1 parent c692757 commit e6850a3c519846d2742f29707a910e977edd5997
@@ -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'
@@ -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('');
@@ -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();
}
}
@@ -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) {
@@ -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', () => {
@@ -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', () => {
@@ -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', () => {
@@ -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', () => {

0 comments on commit e6850a3

Please sign in to comment.
You can’t perform that action at this time.