fix(ngcc): libraries using spread in object literals cannot be processed (#34661)
Consider a library that uses a shared constant for host bindings. e.g. ```ts export const BASE_BINDINGS= { '[class.mat-themed]': '_isThemed', } ---- @Directive({ host: {...BASE_BINDINGS, '(click)': '...'} }) export class Dir1 {} @Directive({ host: {...BASE_BINDINGS, '(click)': '...'} }) export class Dir2 {} ``` Previously when these components were shipped as part of the library to NPM, consumers were able to consume `Dir1` and `Dir2`. No errors showed up. Now with Ivy, when ngcc tries to process the library, an error will be thrown. The error is stating that the host bindings should be an object (which they obviously are). This happens because TypeScript transforms the object spread to individual `Object.assign` calls (for compatibility). The partial evaluator used by the `@Directive` annotation handler is unable to process this expression because there is no integrated support for `Object.assign`. In View Engine, this was not a problem because the `metadata.json` files from the library were used to compute the host bindings. Fixes #34659 PR Close #34661
This commit is contained in:

committed by
Andrew Kushnir

parent
a10d2a8dc4
commit
4eeb6cf24d
@ -114,7 +114,7 @@ export class ModuleWithProvidersAnalyzer {
|
||||
`The referenced NgModule in ${fn.declaration.getText()} is not a named class declaration in the typings program; instead we get ${dtsNgModule.getText()}`);
|
||||
}
|
||||
|
||||
return {node: dtsNgModule, viaModule: null};
|
||||
return {node: dtsNgModule, known: null, viaModule: null};
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -126,6 +126,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
|
||||
name,
|
||||
declaration: {
|
||||
node: null,
|
||||
known: null,
|
||||
expression: exportExpression,
|
||||
viaModule: null,
|
||||
},
|
||||
@ -159,9 +160,10 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
|
||||
const reexports: ExportDeclaration[] = [];
|
||||
importedExports.forEach((decl, name) => {
|
||||
if (decl.node !== null) {
|
||||
reexports.push({name, declaration: {node: decl.node, viaModule}});
|
||||
reexports.push({name, declaration: {node: decl.node, known: null, viaModule}});
|
||||
} else {
|
||||
reexports.push({name, declaration: {node: null, expression: decl.expression, viaModule}});
|
||||
reexports.push(
|
||||
{name, declaration: {node: null, known: null, expression: decl.expression, viaModule}});
|
||||
}
|
||||
});
|
||||
return reexports;
|
||||
@ -186,7 +188,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
|
||||
}
|
||||
|
||||
const viaModule = !importInfo.from.startsWith('.') ? importInfo.from : null;
|
||||
return {node: importedFile, viaModule};
|
||||
return {node: importedFile, known: null, viaModule};
|
||||
}
|
||||
|
||||
private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile
|
||||
|
@ -8,7 +8,7 @@
|
||||
|
||||
import * as ts from 'typescript';
|
||||
|
||||
import {ClassDeclaration, ClassMember, ClassMemberKind, ConcreteDeclaration, CtorParameter, Declaration, Decorator, TypeScriptReflectionHost, TypeValueReference, isDecoratorIdentifier, reflectObjectLiteral} from '../../../src/ngtsc/reflection';
|
||||
import {ClassDeclaration, ClassMember, ClassMemberKind, ConcreteDeclaration, CtorParameter, Declaration, Decorator, KnownDeclaration, TypeScriptReflectionHost, TypeValueReference, isDecoratorIdentifier, reflectObjectLiteral,} from '../../../src/ngtsc/reflection';
|
||||
import {isWithinPackage} from '../analysis/util';
|
||||
import {Logger} from '../logging/logger';
|
||||
import {BundleProgram} from '../packages/bundle_program';
|
||||
@ -353,6 +353,17 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
|
||||
}
|
||||
}
|
||||
|
||||
// If the identifier resolves to the global JavaScript `Object`, return a
|
||||
// declaration that denotes it as the known `JsGlobalObject` declaration.
|
||||
if (superDeclaration !== null && this.isJavaScriptObjectDeclaration(superDeclaration)) {
|
||||
return {
|
||||
known: KnownDeclaration.JsGlobalObject,
|
||||
expression: id,
|
||||
viaModule: null,
|
||||
node: null,
|
||||
};
|
||||
}
|
||||
|
||||
return superDeclaration;
|
||||
}
|
||||
|
||||
@ -1697,6 +1708,30 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
|
||||
const exportDecl = namespaceExports.get(expression.name.text) !;
|
||||
return {...exportDecl, viaModule: namespaceDecl.viaModule};
|
||||
}
|
||||
|
||||
/** Checks if the specified declaration resolves to the known JavaScript global `Object`. */
|
||||
protected isJavaScriptObjectDeclaration(decl: Declaration): boolean {
|
||||
if (decl.node === null) {
|
||||
return false;
|
||||
}
|
||||
const node = decl.node;
|
||||
// The default TypeScript library types the global `Object` variable through
|
||||
// a variable declaration with a type reference resolving to `ObjectConstructor`.
|
||||
if (!ts.isVariableDeclaration(node) || !ts.isIdentifier(node.name) ||
|
||||
node.name.text !== 'Object' || node.type === undefined) {
|
||||
return false;
|
||||
}
|
||||
const typeNode = node.type;
|
||||
// If the variable declaration does not have a type resolving to `ObjectConstructor`,
|
||||
// we cannot guarantee that the declaration resolves to the global `Object` variable.
|
||||
if (!ts.isTypeReferenceNode(typeNode) || !ts.isIdentifier(typeNode.typeName) ||
|
||||
typeNode.typeName.text !== 'ObjectConstructor') {
|
||||
return false;
|
||||
}
|
||||
// Finally, check if the type definition for `Object` originates from a default library
|
||||
// definition file. This requires default types to be enabled for the host program.
|
||||
return this.src.program.isSourceFileDefaultLibrary(node.getSourceFile());
|
||||
}
|
||||
}
|
||||
|
||||
///////////// Exported Helpers /////////////
|
||||
|
@ -14,6 +14,7 @@ import {getNameText, hasNameIdentifier, stripDollarSuffix} from '../utils';
|
||||
import {Esm2015ReflectionHost, ParamInfo, getPropertyValueFromSymbol, isAssignment, isAssignmentStatement} from './esm2015_host';
|
||||
import {NgccClassSymbol} from './ngcc_host';
|
||||
|
||||
|
||||
/**
|
||||
* ESM5 packages contain ECMAScript IIFE functions that act like classes. For example:
|
||||
*
|
||||
@ -655,6 +656,8 @@ function getTsHelperFn(node: ts.NamedDeclaration): TsHelperFn|null {
|
||||
null;
|
||||
|
||||
switch (name) {
|
||||
case '__assign':
|
||||
return TsHelperFn.Assign;
|
||||
case '__spread':
|
||||
return TsHelperFn.Spread;
|
||||
case '__spreadArrays':
|
||||
|
@ -140,6 +140,7 @@ export class UmdReflectionHost extends Esm5ReflectionHost {
|
||||
name,
|
||||
declaration: {
|
||||
node: null,
|
||||
known: null,
|
||||
expression: exportExpression,
|
||||
viaModule: null,
|
||||
},
|
||||
@ -182,9 +183,10 @@ export class UmdReflectionHost extends Esm5ReflectionHost {
|
||||
const reexports: ExportDeclaration[] = [];
|
||||
importedExports.forEach((decl, name) => {
|
||||
if (decl.node !== null) {
|
||||
reexports.push({name, declaration: {node: decl.node, viaModule}});
|
||||
reexports.push({name, declaration: {node: decl.node, known: null, viaModule}});
|
||||
} else {
|
||||
reexports.push({name, declaration: {node: null, expression: decl.expression, viaModule}});
|
||||
reexports.push(
|
||||
{name, declaration: {node: null, known: null, expression: decl.expression, viaModule}});
|
||||
}
|
||||
});
|
||||
return reexports;
|
||||
@ -213,7 +215,7 @@ export class UmdReflectionHost extends Esm5ReflectionHost {
|
||||
|
||||
// We need to add the `viaModule` because the `getExportsOfModule()` call
|
||||
// did not know that we were importing the declaration.
|
||||
return {node: importedFile, viaModule: importInfo.from};
|
||||
return {node: importedFile, known: null, viaModule: importInfo.from};
|
||||
}
|
||||
|
||||
private resolveModuleName(moduleName: string, containingFile: ts.SourceFile): ts.SourceFile
|
||||
|
@ -6,7 +6,7 @@
|
||||
* found in the LICENSE file at https://angular.io/license
|
||||
*/
|
||||
import * as ts from 'typescript';
|
||||
import {AbsoluteFsPath, FileSystem, NgtscCompilerHost, absoluteFrom} from '../../../src/ngtsc/file_system';
|
||||
import {AbsoluteFsPath, FileSystem, NgtscCompilerHost} from '../../../src/ngtsc/file_system';
|
||||
import {PathMappings} from '../utils';
|
||||
import {BundleProgram, makeBundleProgram} from './bundle_program';
|
||||
import {EntryPoint, EntryPointFormat} from './entry_point';
|
||||
@ -50,8 +50,7 @@ export function makeEntryPointBundle(
|
||||
const rootDir = entryPoint.package;
|
||||
const options: ts.CompilerOptions = {
|
||||
allowJs: true,
|
||||
maxNodeModuleJsDepth: Infinity,
|
||||
noLib: true, rootDir, ...pathMappings
|
||||
maxNodeModuleJsDepth: Infinity, rootDir, ...pathMappings
|
||||
};
|
||||
const srcHost = new NgccSourcesCompilerHost(fs, options, entryPoint.path);
|
||||
const dtsHost = new NgtscCompilerHost(fs, options);
|
||||
|
@ -1814,6 +1814,86 @@ runInEachFileSystem(() => {
|
||||
expect(definition.helper).toBe(TsHelperFn.SpreadArrays);
|
||||
expect(definition.parameters.length).toEqual(0);
|
||||
});
|
||||
|
||||
it('should recognize TypeScript __assign helper function declaration', () => {
|
||||
const file: TestFile = {
|
||||
name: _('/declaration.d.ts'),
|
||||
contents: `export declare function __assign(...args: object[]): object;`,
|
||||
};
|
||||
loadTestFiles([file]);
|
||||
const bundle = makeTestBundleProgram(file.name);
|
||||
const host = new Esm5ReflectionHost(new MockLogger(), false, bundle);
|
||||
|
||||
const node =
|
||||
getDeclaration(bundle.program, file.name, '__assign', isNamedFunctionDeclaration) !;
|
||||
|
||||
const definition = host.getDefinitionOfFunction(node) !;
|
||||
expect(definition.node).toBe(node);
|
||||
expect(definition.body).toBeNull();
|
||||
expect(definition.helper).toBe(TsHelperFn.Assign);
|
||||
expect(definition.parameters.length).toEqual(0);
|
||||
});
|
||||
|
||||
it('should recognize TypeScript __assign helper function implementation', () => {
|
||||
const file: TestFile = {
|
||||
name: _('/implementation.js'),
|
||||
contents: `
|
||||
var __assign = (this && this.__assign) || function () {
|
||||
__assign = Object.assign || function(t) {
|
||||
for (var s, i = 1, n = arguments.length; i < n; i++) {
|
||||
s = arguments[i];
|
||||
for (var p in s) if (Object.prototype.hasOwnProperty.call(s, p))
|
||||
t[p] = s[p];
|
||||
}
|
||||
return t;
|
||||
};
|
||||
return __assign.apply(this, arguments);
|
||||
};`,
|
||||
};
|
||||
loadTestFiles([file]);
|
||||
const bundle = makeTestBundleProgram(file.name);
|
||||
const host = new Esm5ReflectionHost(new MockLogger(), false, bundle);
|
||||
|
||||
const node =
|
||||
getDeclaration(bundle.program, file.name, '__assign', ts.isVariableDeclaration) !;
|
||||
|
||||
const definition = host.getDefinitionOfFunction(node) !;
|
||||
expect(definition.node).toBe(node);
|
||||
expect(definition.body).toBeNull();
|
||||
expect(definition.helper).toBe(TsHelperFn.Assign);
|
||||
expect(definition.parameters.length).toEqual(0);
|
||||
});
|
||||
|
||||
it('should recognize TypeScript __assign helper function implementation when suffixed',
|
||||
() => {
|
||||
const file: TestFile = {
|
||||
name: _('/implementation.js'),
|
||||
contents: `
|
||||
var __assign$2 = (this && this.__assign$2) || function () {
|
||||
__assign$2 = Object.assign || function(t) {
|
||||
for (var s, i = 1, n = arguments.length; i < n; i++) {
|
||||
s = arguments[i];
|
||||
for (var p in s) if (Object.prototype.hasOwnProperty.call(s, p))
|
||||
t[p] = s[p];
|
||||
}
|
||||
return t;
|
||||
};
|
||||
return __assign$2.apply(this, arguments);
|
||||
};`,
|
||||
};
|
||||
loadTestFiles([file]);
|
||||
const bundle = makeTestBundleProgram(file.name);
|
||||
const host = new Esm5ReflectionHost(new MockLogger(), false, bundle);
|
||||
|
||||
const node =
|
||||
getDeclaration(bundle.program, file.name, '__assign$2', ts.isVariableDeclaration) !;
|
||||
|
||||
const definition = host.getDefinitionOfFunction(node) !;
|
||||
expect(definition.node).toBe(node);
|
||||
expect(definition.body).toBeNull();
|
||||
expect(definition.helper).toBe(TsHelperFn.Assign);
|
||||
expect(definition.parameters.length).toEqual(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('getImportOfIdentifier()', () => {
|
||||
|
@ -146,6 +146,49 @@ runInEachFileSystem(() => {
|
||||
'{ bar: [{ type: Input }] }); })();');
|
||||
});
|
||||
|
||||
['esm5', 'esm2015'].forEach(target => {
|
||||
it(`should be able to process spread operator inside objects for ${target} format`, () => {
|
||||
compileIntoApf(
|
||||
'test-package', {
|
||||
'/index.ts': `
|
||||
import {Directive, Input, NgModule} from '@angular/core';
|
||||
|
||||
const a = { '[class.a]': 'true' };
|
||||
const b = { '[class.b]': 'true' };
|
||||
|
||||
@Directive({
|
||||
selector: '[foo]',
|
||||
host: {...a, ...b, '[class.c]': 'false'}
|
||||
})
|
||||
export class FooDirective {}
|
||||
|
||||
@NgModule({
|
||||
declarations: [FooDirective],
|
||||
})
|
||||
export class FooModule {}
|
||||
`,
|
||||
},
|
||||
{importHelpers: true});
|
||||
|
||||
// TODO: add test with import helpers disabled. This currently won't work because
|
||||
// inlined TS helper functions are not detected. For more details, see PR:
|
||||
// https://github.com/angular/angular/pull/34169
|
||||
fs.writeFile(
|
||||
_('/node_modules/tslib/index.d.ts'),
|
||||
`export declare function __assign(...args: object[]): object;`);
|
||||
|
||||
mainNgcc({
|
||||
basePath: '/node_modules',
|
||||
targetEntryPointPath: 'test-package',
|
||||
propertiesToConsider: [target],
|
||||
});
|
||||
|
||||
const jsContents = fs.readFile(_(`/node_modules/test-package/${target}/src/index.js`))
|
||||
.replace(/\s+/g, ' ');
|
||||
expect(jsContents).toContain('ngcc0.ɵɵclassProp("a", true)("b", true)("c", false)');
|
||||
});
|
||||
});
|
||||
|
||||
it('should not add `const` in ES5 generated code', () => {
|
||||
compileIntoFlatEs5Package('test-package', {
|
||||
'/index.ts': `
|
||||
|
@ -109,13 +109,15 @@ function compileIntoFlatPackage(
|
||||
* All generated code is written into the `node_modules` in the top-level filesystem, ready for use
|
||||
* in testing ngcc.
|
||||
*/
|
||||
export function compileIntoApf(pkgName: string, sources: PackageSources): void {
|
||||
export function compileIntoApf(
|
||||
pkgName: string, sources: PackageSources, extraCompilerOptions: ts.CompilerOptions = {}): void {
|
||||
const fs = getFileSystem();
|
||||
const {rootNames, compileFs} = setupCompileFs(sources);
|
||||
|
||||
const emit = (options: ts.CompilerOptions) => {
|
||||
const host = new MockCompilerHost(compileFs);
|
||||
const program = ts.createProgram({host, rootNames, options});
|
||||
const program =
|
||||
ts.createProgram({host, rootNames, options: {...extraCompilerOptions, ...options}});
|
||||
program.emit();
|
||||
};
|
||||
|
||||
|
Reference in New Issue
Block a user