cargdev/angular
1
0
Fork 0
Code Issues 6 Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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
This commit is contained in:
Pete Bacon Darwin
2020-01-07 14:40:17 +00:00
committed by Alex Rickabaugh
parent 4def99ed38
commit 83868be713
2 changed files with 85 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts
View File

@ -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', () => {