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 2319171bce..4860cda6b8 100644 --- a/packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts +++ b/packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts @@ -27,7 +27,24 @@ export class UmdRenderingFormatter extends Esm5RenderingFormatter { constructor(protected umdHost: UmdReflectionHost, isCore: boolean) { super(umdHost, isCore); } /** - * Add the imports to the UMD module IIFE. + * Add the imports to the UMD module IIFE. + * + * Note that imports at "prepended" to the start of the parameter list of the factory function, + * and so also to the arguments passed to it when it is called. + * This is because there are scenarios where the factory function does not accept as many + * parameters as are passed as argument in the call. For example: + * + * ``` + * (function (global, factory) { + * typeof exports === 'object' && typeof module !== 'undefined' ? + * factory(exports,require('x'),require('z')) : + * typeof define === 'function' && define.amd ? + * define(['exports', 'x', 'z'], factory) : + * (global = global || self, factory(global.myBundle = {}, global.x)); + * }(this, (function (exports, x) { ... } + * ``` + * + * (See that the `z` import is not being used by the factory function.) */ addImports(output: MagicString, imports: Import[], file: ts.SourceFile): void { if (imports.length === 0) { @@ -125,10 +142,13 @@ function renderCommonJsDependencies( return; } const factoryCall = conditional.whenTrue; - // Backup one char to account for the closing parenthesis on the call - const injectionPoint = factoryCall.getEnd() - 1; + const injectionPoint = factoryCall.arguments.length > 0 ? + // Add extra dependencies before the first argument + factoryCall.arguments[0].getFullStart() : + // Backup one char to account for the closing parenthesis on the call + factoryCall.getEnd() - 1; const importString = imports.map(i => `require('${i.specifier}')`).join(','); - output.appendLeft(injectionPoint, (factoryCall.arguments.length > 0 ? ',' : '') + importString); + output.appendLeft(injectionPoint, importString + (factoryCall.arguments.length > 0 ? ',' : '')); } /** @@ -152,11 +172,14 @@ function renderAmdDependencies( 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; + // Already an array + const injectionPoint = dependencyArray.elements.length > 0 ? + // Add imports before the first item. + dependencyArray.elements[0].getFullStart() : + // Backup one char to account for the closing square bracket on the array + dependencyArray.getEnd() - 1; output.appendLeft( - injectionPoint, (dependencyArray.elements.length > 0 ? ',' : '') + importString); + injectionPoint, importString + (dependencyArray.elements.length > 0 ? ',' : '')); } } @@ -169,11 +192,14 @@ function renderGlobalDependencies( if (!globalFactoryCall) { return; } - // Backup one char to account for the closing parenthesis after the argument list of the call. - const injectionPoint = globalFactoryCall.getEnd() - 1; + const injectionPoint = globalFactoryCall.arguments.length > 0 ? + // Add extra dependencies before the first argument + globalFactoryCall.arguments[0].getFullStart() : + // Backup one char to account for the closing parenthesis on the call + globalFactoryCall.getEnd() - 1; const importString = imports.map(i => `global.${getGlobalIdentifier(i)}`).join(','); output.appendLeft( - injectionPoint, (globalFactoryCall.arguments.length > 0 ? ',' : '') + importString); + injectionPoint, importString + (globalFactoryCall.arguments.length > 0 ? ',' : '')); } /** @@ -197,8 +223,8 @@ function renderFactoryParameters( const parameters = factoryFunction.parameters; const parameterString = imports.map(i => i.qualifier).join(','); if (parameters.length > 0) { - const injectionPoint = parameters[parameters.length - 1].getEnd(); - output.appendLeft(injectionPoint, ',' + parameterString); + const injectionPoint = parameters[0].getFullStart(); + output.appendLeft(injectionPoint, parameterString + ','); } else { // If there are no parameters then the factory function will look like: // function () { ... } 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 07f3e11420..7d5cac130d 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 @@ -205,7 +205,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', file); expect(output.toString()) .toContain( - `typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports,require('some-side-effect'),require('/local-dep'),require('@angular/core'),require('@angular/core'),require('@angular/common')) :`); + `typeof exports === 'object' && typeof module !== 'undefined' ? ` + + `factory(require('@angular/core'),require('@angular/common'),exports,require('some-side-effect'),require('/local-dep'),require('@angular/core')) :`); }); it('should append the given imports into the AMD initialization', () => { @@ -221,7 +222,7 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', file); expect(output.toString()) .toContain( - `typeof define === 'function' && define.amd ? define('file', ['exports','some-side-effect','/local-dep','@angular/core','@angular/core','@angular/common'], factory) :`); + `typeof define === 'function' && define.amd ? define('file', ['@angular/core','@angular/common','exports','some-side-effect','/local-dep','@angular/core'], factory) :`); }); it('should append the given imports into the global initialization', () => { @@ -237,7 +238,7 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', file); expect(output.toString()) .toContain( - `(factory(global.file,global.someSideEffect,global.localDep,global.ng.core,global.ng.core,global.ng.common));`); + `(factory(global.ng.core,global.ng.common,global.file,global.someSideEffect,global.localDep,global.ng.core));`); }); it('should remap import identifiers to valid global properties', () => { @@ -255,8 +256,9 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', file); expect(output.toString()) .toContain( - `(factory(global.file,global.someSideEffect,global.localDep,global.ng.core,` + - `global.ngrx.store,global.ng.platformBrowserDynamic,global.ng.common.testing,global.angularFoo.package));`); + `(factory(` + + `global.ngrx.store,global.ng.platformBrowserDynamic,global.ng.common.testing,global.angularFoo.package,` + + `global.file,global.someSideEffect,global.localDep,global.ng.core));`); }); it('should append the given imports into the global initialization, if it has a global/self initializer', @@ -273,7 +275,7 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', file); expect(output.toString()) .toContain( - `(global = global || self, factory(global.file,global.someSideEffect,global.localDep,global.ng.core,global.ng.core,global.ng.common));`); + `(global = global || self, factory(global.ng.core,global.ng.common,global.file,global.someSideEffect,global.localDep,global.ng.core));`); }); it('should append the given imports as parameters into the factory function definition', @@ -289,7 +291,7 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', ], file); expect(output.toString()) - .toContain(`(function (exports,someSideEffect,localDep,core,i0,i1) {'use strict';`); + .toContain(`(function (i0,i1,exports,someSideEffect,localDep,core) {'use strict';`); }); it('should handle the case where there were no prior imports nor exports', () => { @@ -337,6 +339,43 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', expect(contentsAfter).toBe(contentsBefore); }); + + it('should handle the case where not all dependencies are used by the factory', () => { + const PROGRAM: TestFile = { + name: _('/node_modules/test-package/some/file.js'), + contents: ` + /* A copyright notice */ + /* A copyright notice */ + (function (global, factory) { + typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports,require('/local-dep'),require('@angular/core'),require('some-side-effect')) : + typeof define === 'function' && define.amd ? define('file', ['exports','/local-dep','@angular/core','some-side-effect'], factory) : + (factory(global.file,global.localDep,global.ng.core,global.someSideEffect)); + }(this, (function (exports,localDep,core) {'use strict'; + // Note that someSideEffect is not in the factory function parameter list + })));`, + }; + 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'),exports,require('/local-dep'),require('@angular/core'),require('some-side-effect')) :`); + expect(outputSrc).toContain( + `typeof define === 'function' && define.amd ? define('file', ` + + `['@angular/core','@angular/common','exports','/local-dep','@angular/core','some-side-effect'], factory) :`); + expect(outputSrc).toContain( + `(factory(global.ng.core,global.ng.common,global.file,global.localDep,global.ng.core,global.someSideEffect));`); + expect(outputSrc).toContain(`(function (i0,i1,exports,localDep,core) {'use strict';`); + }); }); describe('addExports', () => {