From 7008efa6dd76f7d09125e3f41f8dcb94c73d6590 Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Sun, 19 May 2019 11:50:40 -0400 Subject: [PATCH 1/3] fix(@ngtools/webpack): downlevel constructor parameter type information The TypeScript `emitDecoratorMetadata` option does not support forward references of ES2015 classes due to the TypeScript compiler transforming the metadata into a direct reference of the yet to be defined class. This results in a runtime TDZ error. This is a known limitation of the `emitDecoratorMetadata` option. The change in the PR removes the need for the option by storing the information in a static property function via the use of the tsickle `ctorParameters` transformation. By leveraging the existing functionality, no changes to the framework code is necessary. Also, minimal new code is required within the CLI, as the relevant tsickle code can be extracted and used with several local modifications. --- .../webpack/src/angular_compiler_plugin.ts | 4 + .../src/transformers/ctor-parameters.ts | 377 ++++++++++++++++++ .../e2e/tests/misc/forwardref-es2105.ts | 46 +++ 3 files changed, 427 insertions(+) create mode 100644 packages/ngtools/webpack/src/transformers/ctor-parameters.ts create mode 100644 tests/legacy-cli/e2e/tests/misc/forwardref-es2105.ts diff --git a/packages/ngtools/webpack/src/angular_compiler_plugin.ts b/packages/ngtools/webpack/src/angular_compiler_plugin.ts index d94bb047e2c7..cf7087d2579a 100644 --- a/packages/ngtools/webpack/src/angular_compiler_plugin.ts +++ b/packages/ngtools/webpack/src/angular_compiler_plugin.ts @@ -60,6 +60,7 @@ import { replaceServerBootstrap, } from './transformers'; import { collectDeepNodes } from './transformers/ast_helpers'; +import { downlevelConstructorParameters } from './transformers/ctor-parameters'; import { AUTO_START_ARG, } from './type_checker'; @@ -922,6 +923,9 @@ export class AngularCompilerPlugin { // Replace resources in JIT. this._transformers.push( replaceResources(isAppPath, getTypeChecker, this._options.directTemplateLoading)); + // Downlevel constructor parameters for DI support + // This is required to support forwardRef in ES2015 due to TDZ issues + this._transformers.push(downlevelConstructorParameters(getTypeChecker)); } else { // Remove unneeded angular decorators. this._transformers.push(removeDecorators(isAppPath, getTypeChecker)); diff --git a/packages/ngtools/webpack/src/transformers/ctor-parameters.ts b/packages/ngtools/webpack/src/transformers/ctor-parameters.ts new file mode 100644 index 000000000000..b43f9e40ddec --- /dev/null +++ b/packages/ngtools/webpack/src/transformers/ctor-parameters.ts @@ -0,0 +1,377 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import * as ts from 'typescript'; + +export function downlevelConstructorParameters( + getTypeChecker: () => ts.TypeChecker, +): ts.TransformerFactory { + return (context: ts.TransformationContext) => { + const transformer = decoratorDownlevelTransformer(getTypeChecker(), []); + + return transformer(context); + }; +} + +// The following is sourced from tsickle with local modifications +// Only the creation of `ctorParameters` is retained +// tslint:disable-next-line:max-line-length +// https://github.com/angular/tsickle/blob/0ceb7d6bc47f6945a6c4c09689f1388eb48f5c07/src/decorator_downlevel_transformer.ts +// + +/** + * Extracts the type of the decorator (the function or expression invoked), as well as all the + * arguments passed to the decorator. Returns an AST with the form: + * + * // For @decorator(arg1, arg2) + * { type: decorator, args: [arg1, arg2] } + */ +function extractMetadataFromSingleDecorator( + decorator: ts.Decorator, + diagnostics: ts.Diagnostic[], +): ts.ObjectLiteralExpression { + const metadataProperties: ts.ObjectLiteralElementLike[] = []; + const expr = decorator.expression; + switch (expr.kind) { + case ts.SyntaxKind.Identifier: + // The decorator was a plain @Foo. + metadataProperties.push(ts.createPropertyAssignment('type', expr)); + break; + case ts.SyntaxKind.CallExpression: + // The decorator was a call, like @Foo(bar). + const call = expr as ts.CallExpression; + metadataProperties.push(ts.createPropertyAssignment('type', call.expression)); + if (call.arguments.length) { + const args: ts.Expression[] = []; + for (const arg of call.arguments) { + args.push(arg); + } + const argsArrayLiteral = ts.createArrayLiteral(args); + argsArrayLiteral.elements.hasTrailingComma = true; + metadataProperties.push(ts.createPropertyAssignment('args', argsArrayLiteral)); + } + break; + default: + diagnostics.push({ + file: decorator.getSourceFile(), + start: decorator.getStart(), + length: decorator.getEnd() - decorator.getStart(), + messageText: `${ + ts.SyntaxKind[decorator.kind] + } not implemented in gathering decorator metadata`, + category: ts.DiagnosticCategory.Error, + code: 0, + }); + break; + } + + return ts.createObjectLiteral(metadataProperties); +} + +/** + * createCtorParametersClassProperty creates a static 'ctorParameters' property containing + * downleveled decorator information. + * + * The property contains an arrow function that returns an array of object literals of the shape: + * static ctorParameters = () => [{ + * type: SomeClass|undefined, // the type of the param that's decorated, if it's a value. + * decorators: [{ + * type: DecoratorFn, // the type of the decorator that's invoked. + * args: [ARGS], // the arguments passed to the decorator. + * }] + * }]; + */ +function createCtorParametersClassProperty( + diagnostics: ts.Diagnostic[], + entityNameToExpression: (n: ts.EntityName) => ts.Expression | undefined, + ctorParameters: ParameterDecorationInfo[], +): ts.PropertyDeclaration { + const params: ts.Expression[] = []; + + for (const ctorParam of ctorParameters) { + if (!ctorParam.type && ctorParam.decorators.length === 0) { + params.push(ts.createNull()); + continue; + } + + const paramType = ctorParam.type + ? typeReferenceToExpression(entityNameToExpression, ctorParam.type) + : undefined; + const members = [ + ts.createPropertyAssignment('type', paramType || ts.createIdentifier('undefined')), + ]; + + const decorators: ts.ObjectLiteralExpression[] = []; + for (const deco of ctorParam.decorators) { + decorators.push(extractMetadataFromSingleDecorator(deco, diagnostics)); + } + if (decorators.length) { + members.push(ts.createPropertyAssignment('decorators', ts.createArrayLiteral(decorators))); + } + params.push(ts.createObjectLiteral(members)); + } + + const initializer = ts.createArrowFunction( + undefined, + undefined, + [], + undefined, + ts.createToken(ts.SyntaxKind.EqualsGreaterThanToken), + ts.createArrayLiteral(params, true), + ); + + const ctorProp = ts.createProperty( + undefined, + [ts.createToken(ts.SyntaxKind.StaticKeyword)], + 'ctorParameters', + undefined, + undefined, + initializer, + ); + + return ctorProp; +} + +/** + * Returns an expression representing the (potentially) value part for the given node. + * + * This is a partial re-implementation of TypeScript's serializeTypeReferenceNode. This is a + * workaround for https://github.com/Microsoft/TypeScript/issues/17516 (serializeTypeReferenceNode + * not being exposed). In practice this implementation is sufficient for Angular's use of type + * metadata. + */ +function typeReferenceToExpression( + entityNameToExpression: (n: ts.EntityName) => ts.Expression | undefined, + node: ts.TypeNode, +): ts.Expression | undefined { + let kind = node.kind; + if (ts.isLiteralTypeNode(node)) { + // Treat literal types like their base type (boolean, string, number). + kind = node.literal.kind; + } + switch (kind) { + case ts.SyntaxKind.FunctionType: + case ts.SyntaxKind.ConstructorType: + return ts.createIdentifier('Function'); + case ts.SyntaxKind.ArrayType: + case ts.SyntaxKind.TupleType: + return ts.createIdentifier('Array'); + case ts.SyntaxKind.TypePredicate: + case ts.SyntaxKind.TrueKeyword: + case ts.SyntaxKind.FalseKeyword: + case ts.SyntaxKind.BooleanKeyword: + return ts.createIdentifier('Boolean'); + case ts.SyntaxKind.StringLiteral: + case ts.SyntaxKind.StringKeyword: + return ts.createIdentifier('String'); + case ts.SyntaxKind.ObjectKeyword: + return ts.createIdentifier('Object'); + case ts.SyntaxKind.NumberKeyword: + case ts.SyntaxKind.NumericLiteral: + return ts.createIdentifier('Number'); + case ts.SyntaxKind.TypeReference: + const typeRef = node as ts.TypeReferenceNode; + + // Ignore any generic types, just return the base type. + return entityNameToExpression(typeRef.typeName); + default: + return undefined; + } +} + +/** ParameterDecorationInfo describes the information for a single constructor parameter. */ +interface ParameterDecorationInfo { + /** + * The type declaration for the parameter. Only set if the type is a value (e.g. a class, not an + * interface). + */ + type: ts.TypeNode | null; + /** The list of decorators found on the parameter, null if none. */ + decorators: ts.Decorator[]; +} + +/** + * Transformer factory for the decorator downlevel transformer. See fileoverview for details. + */ +export function decoratorDownlevelTransformer( + typeChecker: ts.TypeChecker, + diagnostics: ts.Diagnostic[], +): (context: ts.TransformationContext) => ts.Transformer { + return (context: ts.TransformationContext) => { + /** A map from symbols to the identifier of an import, reset per SourceFile. */ + let importNamesBySymbol = new Map(); + + /** + * Converts an EntityName (from a type annotation) to an expression (accessing a value). + * + * For a given ts.EntityName, this walks depth first to find the leftmost ts.Identifier, then + * converts the path into property accesses. + * + * This generally works, but TypeScript's emit pipeline does not serialize identifiers that are + * only used in a type location (such as identifiers in a TypeNode), even if the identifier + * itself points to a value (e.g. a class). To avoid that problem, this method finds the symbol + * representing the identifier (using typeChecker), then looks up where it was imported (using + * importNamesBySymbol), and then uses the imported name instead of the identifier from the type + * expression, if any. Otherwise it'll use the identifier unchanged. This makes sure the + * identifier is not marked as stemming from a "type only" expression, causing it to be emitted + * and causing the import to be retained. + */ + function entityNameToExpression(name: ts.EntityName): ts.Expression | undefined { + const sym = typeChecker.getSymbolAtLocation(name); + if (!sym) { + return undefined; + } + // Check if the entity name references a symbol that is an actual value. If it is not, it + // cannot be referenced by an expression, so return undefined. + let symToCheck = sym; + if (symToCheck.flags & ts.SymbolFlags.Alias) { + symToCheck = typeChecker.getAliasedSymbol(symToCheck); + } + if (!(symToCheck.flags & ts.SymbolFlags.Value)) { + return undefined; + } + + if (ts.isIdentifier(name)) { + // If there's a known import name for this symbol, use it so that the import will be + // retained and the value can be referenced. + const value = importNamesBySymbol.get(sym); + if (value) { + return value; + } + + // Otherwise this will be a locally declared name, just return that. + return name; + } + const ref = entityNameToExpression(name.left); + if (!ref) { + return undefined; + } + + return ts.createPropertyAccess(ref, name.right); + } + + /** + * Transforms a constructor. Returns the transformed constructor and the list of parameter + * information collected, consisting of decorators and optional type. + */ + function transformConstructor( + ctor: ts.ConstructorDeclaration, + ): [ts.ConstructorDeclaration, ParameterDecorationInfo[]] { + ctor = ts.visitEachChild(ctor, visitor, context); + + const parametersInfo: ParameterDecorationInfo[] = []; + for (const param of ctor.parameters) { + const paramInfo: ParameterDecorationInfo = { decorators: [], type: null }; + + for (const decorator of param.decorators || []) { + paramInfo.decorators.push(decorator); + } + if (param.type) { + // param has a type provided, e.g. "foo: Bar". + // The type will be emitted as a value expression in entityNameToExpression, which takes + // care not to emit anything for types that cannot be expressed as a value (e.g. + // interfaces). + paramInfo.type = param.type; + } + parametersInfo.push(paramInfo); + } + + return [ctor, parametersInfo]; + } + + /** + * Transforms a single class declaration: + * - creates a ctorParameters property + */ + function transformClassDeclaration(classDecl: ts.ClassDeclaration): ts.ClassDeclaration { + if (!classDecl.decorators || classDecl.decorators.length === 0) { + return classDecl; + } + + const newMembers: ts.ClassElement[] = []; + let classParameters: ParameterDecorationInfo[] | null = null; + + for (const member of classDecl.members) { + switch (member.kind) { + case ts.SyntaxKind.Constructor: { + const ctor = member as ts.ConstructorDeclaration; + if (!ctor.body) { + break; + } + + const [newMember, parametersInfo] = transformConstructor( + member as ts.ConstructorDeclaration, + ); + classParameters = parametersInfo; + newMembers.push(newMember); + continue; + } + default: + break; + } + newMembers.push(ts.visitEachChild(member, visitor, context)); + } + + const newClassDeclaration = ts.getMutableClone(classDecl); + + if (classParameters) { + newMembers.push( + createCtorParametersClassProperty(diagnostics, entityNameToExpression, classParameters), + ); + } + + newClassDeclaration.members = ts.setTextRange( + ts.createNodeArray(newMembers, newClassDeclaration.members.hasTrailingComma), + classDecl.members, + ); + + return newClassDeclaration; + } + + function visitor(node: ts.Node): ts.Node { + switch (node.kind) { + case ts.SyntaxKind.SourceFile: { + importNamesBySymbol = new Map(); + + return ts.visitEachChild(node, visitor, context); + } + case ts.SyntaxKind.ImportDeclaration: { + const impDecl = node as ts.ImportDeclaration; + if (impDecl.importClause) { + const importClause = impDecl.importClause; + const names = []; + if (importClause.name) { + names.push(importClause.name); + } + if ( + importClause.namedBindings && + importClause.namedBindings.kind === ts.SyntaxKind.NamedImports + ) { + const namedImports = importClause.namedBindings as ts.NamedImports; + names.push(...namedImports.elements.map(e => e.name)); + } + for (const name of names) { + const sym = typeChecker.getSymbolAtLocation(name); + if (sym) { + importNamesBySymbol.set(sym, name); + } + } + } + + return ts.visitEachChild(node, visitor, context); + } + case ts.SyntaxKind.ClassDeclaration: { + return transformClassDeclaration(node as ts.ClassDeclaration); + } + default: + return ts.visitEachChild(node, visitor, context); + } + } + + return (sf: ts.SourceFile) => visitor(sf) as ts.SourceFile; + }; +} diff --git a/tests/legacy-cli/e2e/tests/misc/forwardref-es2105.ts b/tests/legacy-cli/e2e/tests/misc/forwardref-es2105.ts new file mode 100644 index 000000000000..af0e32ee5c01 --- /dev/null +++ b/tests/legacy-cli/e2e/tests/misc/forwardref-es2105.ts @@ -0,0 +1,46 @@ +import { appendToFile, replaceInFile, writeFile } from '../../utils/fs'; +import { ng } from '../../utils/process'; +import { expectToFail } from '../../utils/utils'; + +export default async function() { + // Ensure an ES2015 build is used in test + await writeFile('browserslist', 'Chrome 65'); + + // Update the application to use a forward reference + await replaceInFile( + 'src/app/app.component.ts', + 'import { Component } from \'@angular/core\';', + 'import { Component, Inject, Injectable, forwardRef } from \'@angular/core\';', + ); + await appendToFile('src/app/app.component.ts', '\n@Injectable() export class Lock { }\n'); + await replaceInFile( + 'src/app/app.component.ts', + 'export class AppComponent {', + 'export class AppComponent {\n constructor(@Inject(forwardRef(() => Lock)) lock: Lock) {}', + ); + + // Update the application's unit tests to include the new injectable + await replaceInFile( + 'src/app/app.component.spec.ts', + 'import { AppComponent } from \'./app.component\';', + 'import { AppComponent, Lock } from \'./app.component\';', + ); + await replaceInFile( + 'src/app/app.component.spec.ts', + 'TestBed.configureTestingModule({', + 'TestBed.configureTestingModule({ providers: [Lock],', + ); + + // Execute the application's tests with emitDecoratorMetadata disabled (default) + await ng('test', '--no-watch'); + + // Turn on emitDecoratorMetadata + await replaceInFile( + 'tsconfig.json', + '"experimentalDecorators": true', + '"experimentalDecorators": true, "emitDecoratorMetadata": true', + ); + + // Execute the application's tests with emitDecoratorMetadata enabled + await expectToFail(() => ng('test', '--no-watch')); +} From 203cf783d34f76a2a9bc7ddd7c70e4f29ab204ea Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Mon, 20 May 2019 20:11:04 -0400 Subject: [PATCH 2/3] fix(@schematics/angular): remove `emitDecoratorMetadata` TS option for new applications --- .../schematics/angular/library/files/tsconfig.lib.json.template | 1 + .../schematics/angular/workspace/files/tsconfig.json.template | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/schematics/angular/library/files/tsconfig.lib.json.template b/packages/schematics/angular/library/files/tsconfig.lib.json.template index 730ff84bbeaf..30e37f49ec97 100644 --- a/packages/schematics/angular/library/files/tsconfig.lib.json.template +++ b/packages/schematics/angular/library/files/tsconfig.lib.json.template @@ -3,6 +3,7 @@ "compilerOptions": { "outDir": "<%= relativePathToWorkspaceRoot %>/out-tsc/lib", "target": "es2015", + "emitDecoratorMetadata": true, "declaration": true, "inlineSources": true, "types": [], diff --git a/packages/schematics/angular/workspace/files/tsconfig.json.template b/packages/schematics/angular/workspace/files/tsconfig.json.template index 6ec9ceb17451..1e35a04fa981 100644 --- a/packages/schematics/angular/workspace/files/tsconfig.json.template +++ b/packages/schematics/angular/workspace/files/tsconfig.json.template @@ -7,7 +7,6 @@ "declaration": false, "module": "esnext", "moduleResolution": "node", - "emitDecoratorMetadata": true, "experimentalDecorators": true, "importHelpers": true, "target": "es2015", From 239b216e1fc90901ba3aa03730c7590d3332a1db Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Tue, 21 May 2019 11:31:31 -0400 Subject: [PATCH 3/3] fix(@schematics/angular): enable tsickle for library compilation This is required to support forward references in ES2015 target code. tsickle provides the constructor parameter downlevel logic that removes the runtime TDZ error that would otherwise be encountered. --- .../angular/library/files/tsconfig.lib.json.template | 2 +- packages/schematics/angular/library/index.ts | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/schematics/angular/library/files/tsconfig.lib.json.template b/packages/schematics/angular/library/files/tsconfig.lib.json.template index 30e37f49ec97..e40a3d9124bf 100644 --- a/packages/schematics/angular/library/files/tsconfig.lib.json.template +++ b/packages/schematics/angular/library/files/tsconfig.lib.json.template @@ -3,7 +3,6 @@ "compilerOptions": { "outDir": "<%= relativePathToWorkspaceRoot %>/out-tsc/lib", "target": "es2015", - "emitDecoratorMetadata": true, "declaration": true, "inlineSources": true, "types": [], @@ -13,6 +12,7 @@ ] }, "angularCompilerOptions": { + "annotateForClosureCompiler": true, "skipTemplateCodegen": true, "strictMetadataEmit": true, "fullTemplateTypeCheck": true, diff --git a/packages/schematics/angular/library/index.ts b/packages/schematics/angular/library/index.ts index 839fb7413860..07dd260fc627 100644 --- a/packages/schematics/angular/library/index.ts +++ b/packages/schematics/angular/library/index.ts @@ -103,6 +103,11 @@ function addDependenciesToPackageJson() { name: 'ng-packagr', version: '^5.1.0', }, + { + type: NodeDependencyType.Dev, + name: 'tsickle', + version: '^0.35.0', + }, { type: NodeDependencyType.Default, name: 'tslib',