Skip to content

Commit

Permalink
fix(ngcc): render UMD global imports correctly (#34012)
Browse files Browse the repository at this point in the history
The current UMD rendering formatter did not handle
a number of corner cases, such as imports from namespaced
packages.

PR Close #34012
  • Loading branch information
petebacondarwin authored and matsko committed Nov 25, 2019
1 parent 290739f commit 83989b8
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,34 @@ function isCommaExpression(value: ts.Node): value is ts.BinaryExpression {
return ts.isBinaryExpression(value) && value.operatorToken.kind === ts.SyntaxKind.CommaToken;
}

function getGlobalIdentifier(i: Import) {
return i.specifier.replace('@angular/', 'ng.').replace(/^\//, '');
/**
* Compute a global identifier for the given import (`i`).
*
* The identifier used to access a package when using the "global" form of a UMD bundle usually
* follows a special format where snake-case is conveted to camelCase and path separators are
* converted to dots. In addition there are special cases such as `@angular` is mapped to `ng`.
*
* For example
*
* * `@ns/package/entry-point` => `ns.package.entryPoint`
* * `@angular/common/testing` => `ng.common.testing`
* * `@angular/platform-browser-dynamic` => `ng.platformBrowserDynamic`
*
* It is possible for packages to specify completely different identifiers for attaching the package
* to the global, and so there is no guaranteed way to compute this.
* Currently, this approach appears to work for the known scenarios; also it is not known how common
* it is to use globals for importing packages.
*
* If it turns out that there are packages that are being used via globals, where this approach
* fails, we should consider implementing a configuration based solution, similar to what would go
* in a rollup configuration for mapping import paths to global indentifiers.
*/
function getGlobalIdentifier(i: Import): string {
return i.specifier.replace(/^@angular\//, 'ng.')
.replace(/^@/, '')
.replace(/\//g, '.')
.replace(/[-_]+(.?)/g, (_, c) => c.toUpperCase())
.replace(/^./, c => c.toLowerCase());
}

function find<T>(node: ts.Node, test: (node: ts.Node) => node is ts.Node & T): T|undefined {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,25 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
`(factory(global.file,global.someSideEffect,global.localDep,global.ng.core,global.ng.core,global.ng.common));`);
});

it('should remap import identifiers to valid global properties', () => {
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: '@ngrx/store', qualifier: 'i0'},
{specifier: '@angular/platform-browser-dynamic', qualifier: 'i1'},
{specifier: '@angular/common/testing', qualifier: 'i2'},
{specifier: '@angular-foo/package', qualifier: 'i3'}
],
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));`);
});

it('should append the given imports into the global initialization, if it has a global/self initializer',
() => {
const {renderer, program} = setup(PROGRAM_WITH_GLOBAL_INITIALIZER);
Expand All @@ -256,6 +275,7 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
.toContain(
`(global = global || self, factory(global.file,global.someSideEffect,global.localDep,global.ng.core,global.ng.core,global.ng.common));`);
});

it('should append the given imports as parameters into the factory function definition',
() => {
const {renderer, program} = setup(PROGRAM);
Expand Down

0 comments on commit 83989b8

Please sign in to comment.