Skip to content
Permalink
Browse files

fix(ngcc): render UMD imports even if no prior imports (#34353)

Previously the UMD rendering formatter assumed that
there would already be import (and an export) arguments
to the UMD factory function.

This commit adds support for this corner case.

Fixes #34138

PR Close #34353
  • Loading branch information
petebacondarwin authored and kara committed Dec 11, 2019
1 parent 241f948 commit c26738df1234e25925d7bd9fe8eca1ebd92fe07e
@@ -121,9 +121,10 @@ function renderCommonJsDependencies(
return;
}
const factoryCall = conditional.whenTrue;
const injectionPoint = factoryCall.getEnd() -
1; // Backup one char to account for the closing parenthesis on the call
imports.forEach(i => output.appendLeft(injectionPoint, `,require('${i.specifier}')`));
// Backup one char to account for the closing parenthesis on the call
const injectionPoint = factoryCall.getEnd() - 1;
const importString = imports.map(i => `require('${i.specifier}')`).join(',');
output.appendLeft(injectionPoint, (factoryCall.arguments.length > 0 ? ',' : '') + importString);
}

/**
@@ -135,13 +136,24 @@ function renderAmdDependencies(
if (!conditional) {
return;
}
const dependencyArray = conditional.whenTrue.arguments[1];
if (!dependencyArray || !ts.isArrayLiteralExpression(dependencyArray)) {
return;
const amdDefineCall = conditional.whenTrue;
const importString = imports.map(i => `'${i.specifier}'`).join(',');
// The dependency array (if it exists) is the second to last argument
// `define(id?, dependencies?, factory);`
const factoryIndex = amdDefineCall.arguments.length - 1;
const dependencyArray = amdDefineCall.arguments[factoryIndex - 1];
if (dependencyArray === undefined || !ts.isArrayLiteralExpression(dependencyArray)) {
// No array provided: `define(factory)` or `define(id, factory)`.
// Insert a new array in front the `factory` call.
const injectionPoint = amdDefineCall.arguments[factoryIndex].getFullStart();
output.appendLeft(injectionPoint, `[${importString}],`);
} else {
// Already an array, add imports to the end of the array.
// Backup one char to account for the closing square bracket on the array
const injectionPoint = dependencyArray.getEnd() - 1;
output.appendLeft(
injectionPoint, (dependencyArray.elements.length > 0 ? ',' : '') + importString);
}
const injectionPoint = dependencyArray.getEnd() -
1; // Backup one char to account for the closing square bracket on the array
imports.forEach(i => output.appendLeft(injectionPoint, `,'${i.specifier}'`));
}

/**
@@ -155,7 +167,9 @@ function renderGlobalDependencies(
}
// Backup one char to account for the closing parenthesis after the argument list of the call.
const injectionPoint = globalFactoryCall.getEnd() - 1;
imports.forEach(i => output.appendLeft(injectionPoint, `,global.${getGlobalIdentifier(i)}`));
const importString = imports.map(i => `global.${getGlobalIdentifier(i)}`).join(',');
output.appendLeft(
injectionPoint, (globalFactoryCall.arguments.length > 0 ? ',' : '') + importString);
}

/**
@@ -175,9 +189,20 @@ function renderFactoryParameters(
if (!ts.isFunctionExpression(factoryFunction)) {
return;
}

const parameters = factoryFunction.parameters;
const injectionPoint = parameters[parameters.length - 1].getEnd();
imports.forEach(i => output.appendLeft(injectionPoint, `,${i.qualifier}`));
const parameterString = imports.map(i => i.qualifier).join(',');
if (parameters.length > 0) {
const injectionPoint = parameters[parameters.length - 1].getEnd();
output.appendLeft(injectionPoint, ',' + parameterString);
} else {
// If there are no parameters then the factory function will look like:
// function () { ... }
// The AST does not give us a way to find the insertion point - between the two parentheses.
// So we must use a regular expression on the text of the function.
const injectionPoint = factoryFunction.getStart() + factoryFunction.getText().indexOf('()') + 1;
output.appendLeft(injectionPoint, parameterString);
}
}

/**
@@ -291,6 +291,40 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
expect(output.toString())
.toContain(`(function (exports,someSideEffect,localDep,core,i0,i1) {'use strict';`);
});

it('should handle the case where there were no prior imports nor exports', () => {
const PROGRAM: TestFile = {
name: _('/node_modules/test-package/some/file.js'),
contents: `
/* A copyright notice */
(function (global, factory) {
typeof exports === 'object' && typeof module !== 'undefined' ? factory() :
typeof define === 'function' && define.amd ? define('file', factory) :
(factory());
}(this, (function () {'use strict';
var index = '';
return index;
})));`,
};
const {renderer, program} = setup(PROGRAM);
const file = getSourceFileOrError(program, _('/node_modules/test-package/some/file.js'));
const output = new MagicString(PROGRAM.contents);
renderer.addImports(
output,
[
{specifier: '@angular/core', qualifier: 'i0'},
{specifier: '@angular/common', qualifier: 'i1'}
],
file);
const outputSrc = output.toString();

expect(outputSrc).toContain(
`typeof exports === 'object' && typeof module !== 'undefined' ? factory(require('@angular/core'),require('@angular/common')) :`);
expect(outputSrc).toContain(
`typeof define === 'function' && define.amd ? define('file',['@angular/core','@angular/common'], factory) :`);
expect(outputSrc).toContain(`(factory(global.ng.core,global.ng.common));`);
expect(outputSrc).toContain(`(function (i0,i1) {'use strict';`);
});
});

describe('addExports', () => {

0 comments on commit c26738d

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