fix(ngcc): handle UMD factories that do not use all params (#34660)

In some cases, where a module imports a dependency
but does not actually use it, UMD bundlers may remove
the dependency parameter from the UMD factory function
definition.

For example:

```
import * as x from 'x';
import * as z from 'z';
export const y = x;
```

may result in a UMD bundle including:

```
(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) { 'use strict';
...
})));
```

Note that while the `z` dependency is provide in the call,
the factory itself only accepts `exports` and `x` as parameters.

Previously ngcc appended new dependencies to the end of the factory
function, but this breaks in the above scenario. Now the new
dependencies are prefixed at the front of parameters/arguments
already in place.

Fixes #34653

PR Close #34660

packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts

@@ -28,6 +28,23 @@ export class UmdRenderingFormatter extends Esm5RenderingFormatter {
 
   /**
    * 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;
+  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
-  const injectionPoint = factoryCall.getEnd() - 1;
+      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.
+    // 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
-    const injectionPoint = dependencyArray.getEnd() - 1;
+        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.arguments.length > 0 ?
-  const injectionPoint = globalFactoryCall.getEnd() - 1;
+      // 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();
+    const injectionPoint = parameters[0].getFullStart();
-    output.appendLeft(injectionPoint, ',' + parameterString);
+    output.appendLeft(injectionPoint, parameterString + ',');
   } else {
     // If there are no parameters then the factory function will look like:
     // function () { ... }

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,` +
+                `(factory(` +
-                `global.ngrx.store,global.ng.platformBrowserDynamic,global.ng.common.testing,global.angularFoo.package));`);
+                `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', () => {