From adfc55e2c3c70949b7127e376ba23ae28257be2e Mon Sep 17 00:00:00 2001 From: JoostK Date: Fri, 25 Jan 2019 03:04:01 +0100 Subject: [PATCH] fix(ivy): ngcc - recognize accessor members in ES5 (#28357) Prior to this change, accessor functions for getters and setters would not be considered as class member, as their declaration is vastly different from ES2015 syntax. With this change, the ES5 reflection host has learned to recognize the downleveled syntax for accessors, allowing for them to be considered as class member once again. Fixes #28226 PR Close #28357 --- .../src/ngcc/src/host/esm2015_host.ts | 26 ++-- .../src/ngcc/src/host/esm5_host.ts | 125 ++++++++++++++++-- .../src/ngcc/test/host/esm5_host_spec.ts | 95 ++++++++++++- 3 files changed, 220 insertions(+), 26 deletions(-) diff --git a/packages/compiler-cli/src/ngcc/src/host/esm2015_host.ts b/packages/compiler-cli/src/ngcc/src/host/esm2015_host.ts index 7d9c9b5c45..7ddb1005bf 100644 --- a/packages/compiler-cli/src/ngcc/src/host/esm2015_host.ts +++ b/packages/compiler-cli/src/ngcc/src/host/esm2015_host.ts @@ -103,9 +103,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N if (symbol.members) { symbol.members.forEach((value, key) => { const decorators = removeFromMap(decoratorsMap, key); - const member = this.reflectMember(value, decorators); - if (member) { - members.push(member); + const reflectedMembers = this.reflectMembers(value, decorators); + if (reflectedMembers) { + members.push(...reflectedMembers); } }); } @@ -114,9 +114,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N if (symbol.exports) { symbol.exports.forEach((value, key) => { const decorators = removeFromMap(decoratorsMap, key); - const member = this.reflectMember(value, decorators, true); - if (member) { - members.push(member); + const reflectedMembers = this.reflectMembers(value, decorators, true); + if (reflectedMembers) { + members.push(...reflectedMembers); } }); } @@ -135,9 +135,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N if (variableSymbol && variableSymbol.exports) { variableSymbol.exports.forEach((value, key) => { const decorators = removeFromMap(decoratorsMap, key); - const member = this.reflectMember(value, decorators, true); - if (member) { - members.push(member); + const reflectedMembers = this.reflectMembers(value, decorators, true); + if (reflectedMembers) { + members.push(...reflectedMembers); } }); } @@ -724,8 +724,8 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N * @param isStatic true if this member is static, false if it is an instance property. * @returns the reflected member information, or null if the symbol is not a member. */ - protected reflectMember(symbol: ts.Symbol, decorators?: Decorator[], isStatic?: boolean): - ClassMember|null { + protected reflectMembers(symbol: ts.Symbol, decorators?: Decorator[], isStatic?: boolean): + ClassMember[]|null { let kind: ClassMemberKind|null = null; let value: ts.Expression|null = null; let name: string|null = null; @@ -783,11 +783,11 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N } const type: ts.TypeNode = (node as any).type || null; - return { + return [{ node, implementation: node, kind, type, name, nameNode, value, isStatic, decorators: decorators || [] - }; + }]; } /** diff --git a/packages/compiler-cli/src/ngcc/src/host/esm5_host.ts b/packages/compiler-cli/src/ngcc/src/host/esm5_host.ts index db04510eaa..8079e290c3 100644 --- a/packages/compiler-cli/src/ngcc/src/host/esm5_host.ts +++ b/packages/compiler-cli/src/ngcc/src/host/esm5_host.ts @@ -208,24 +208,69 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost { /** * Reflect over a symbol and extract the member information, combining it with the * provided decorator information, and whether it is a static member. + * + * If a class member uses accessors (e.g getters and/or setters) then it gets downleveled + * in ES5 to a single `Object.defineProperty()` call. In that case we must parse this + * call to extract the one or two ClassMember objects that represent the accessors. + * * @param symbol the symbol for the member to reflect over. * @param decorators an array of decorators associated with the member. * @param isStatic true if this member is static, false if it is an instance property. * @returns the reflected member information, or null if the symbol is not a member. */ - protected reflectMember(symbol: ts.Symbol, decorators?: Decorator[], isStatic?: boolean): - ClassMember|null { - const member = super.reflectMember(symbol, decorators, isStatic); - if (member && member.kind === ClassMemberKind.Method && member.isStatic && member.node && - ts.isPropertyAccessExpression(member.node) && member.node.parent && - ts.isBinaryExpression(member.node.parent) && - ts.isFunctionExpression(member.node.parent.right)) { - // Recompute the implementation for this member: - // ES5 static methods are variable declarations so the declaration is actually the - // initializer of the variable assignment - member.implementation = member.node.parent.right; + protected reflectMembers(symbol: ts.Symbol, decorators?: Decorator[], isStatic?: boolean): + ClassMember[]|null { + const node = symbol.valueDeclaration || symbol.declarations && symbol.declarations[0]; + const propertyDefinition = getPropertyDefinition(node); + if (propertyDefinition) { + const members: ClassMember[] = []; + if (propertyDefinition.setter) { + members.push({ + node, + implementation: propertyDefinition.setter, + kind: ClassMemberKind.Setter, + type: null, + name: symbol.name, + nameNode: null, + value: null, + isStatic: isStatic || false, + decorators: decorators || [], + }); + + // Prevent attaching the decorators to a potential getter. In ES5, we can't tell where the + // decorators were originally attached to, however we only want to attach them to a single + // `ClassMember` as otherwise ngtsc would handle the same decorators twice. + decorators = undefined; + } + if (propertyDefinition.getter) { + members.push({ + node, + implementation: propertyDefinition.getter, + kind: ClassMemberKind.Getter, + type: null, + name: symbol.name, + nameNode: null, + value: null, + isStatic: isStatic || false, + decorators: decorators || [], + }); + } + return members; } - return member; + + const members = super.reflectMembers(symbol, decorators, isStatic); + members && members.forEach(member => { + if (member && member.kind === ClassMemberKind.Method && member.isStatic && member.node && + ts.isPropertyAccessExpression(member.node) && member.node.parent && + ts.isBinaryExpression(member.node.parent) && + ts.isFunctionExpression(member.node.parent.right)) { + // Recompute the implementation for this member: + // ES5 static methods are variable declarations so the declaration is actually the + // initializer of the variable assignment + member.implementation = member.node.parent.right; + } + }); + return members; } /** @@ -246,6 +291,62 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost { ///////////// Internal Helpers ///////////// +/** + * Represents the details about property definitions that were set using `Object.defineProperty`. + */ +interface PropertyDefinition { + setter: ts.FunctionExpression|null; + getter: ts.FunctionExpression|null; +} + +/** + * In ES5, getters and setters have been downleveled into call expressions of + * `Object.defineProperty`, such as + * + * ``` + * Object.defineProperty(Clazz.prototype, "property", { + * get: function () { + * return 'value'; + * }, + * set: function (value) { + * this.value = value; + * }, + * enumerable: true, + * configurable: true + * }); + * ``` + * + * This function inspects the given node to determine if it corresponds with such a call, and if so + * extracts the `set` and `get` function expressions from the descriptor object, if they exist. + * + * @param node The node to obtain the property definition from. + * @returns The property definition if the node corresponds with accessor, null otherwise. + */ +function getPropertyDefinition(node: ts.Node): PropertyDefinition|null { + if (!ts.isCallExpression(node)) return null; + + const fn = node.expression; + if (!ts.isPropertyAccessExpression(fn) || !ts.isIdentifier(fn.expression) || + fn.expression.text !== 'Object' || fn.name.text !== 'defineProperty') + return null; + + const descriptor = node.arguments[2]; + if (!descriptor || !ts.isObjectLiteralExpression(descriptor)) return null; + + return { + setter: readPropertyFunctionExpression(descriptor, 'set'), + getter: readPropertyFunctionExpression(descriptor, 'get'), + }; +} + +function readPropertyFunctionExpression(object: ts.ObjectLiteralExpression, name: string) { + const property = object.properties.find( + (p): p is ts.PropertyAssignment => + ts.isPropertyAssignment(p) && ts.isIdentifier(p.name) && p.name.text === name); + + return property && ts.isFunctionExpression(property.initializer) && property.initializer || null; +} + function getIifeBody(declaration: ts.VariableDeclaration): ts.Block|undefined { if (!declaration.initializer || !ts.isParenthesizedExpression(declaration.initializer)) { return undefined; diff --git a/packages/compiler-cli/src/ngcc/test/host/esm5_host_spec.ts b/packages/compiler-cli/src/ngcc/test/host/esm5_host_spec.ts index 1501869fc9..6a3cfcd69c 100644 --- a/packages/compiler-cli/src/ngcc/test/host/esm5_host_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/host/esm5_host_spec.ts @@ -47,6 +47,52 @@ const SOME_DIRECTIVE_FILE = { }()); `, }; +const ACCESSORS_FILE = { + name: '/accessors.js', + contents: ` + import { Directive, Input, Output } from '@angular/core'; + + var SomeDirective = (function() { + function SomeDirective() { + } + Object.defineProperty(SomeDirective.prototype, "setter", { + set: function (value) { this.value = value; }, + enumerable: true, + configurable: true + }); + Object.defineProperty(SomeDirective.prototype, "getter", { + get: function () { return null; }, + enumerable: true, + configurable: true + }); + Object.defineProperty(SomeDirective.prototype, "setterAndGetter", { + get: function () { return null; }, + set: function (value) { this.value = value; }, + enumerable: true, + configurable: true + }); + Object.defineProperty(SomeDirective, "staticSetter", { + set: function (value) { this.value = value; }, + enumerable: true, + configurable: true + }); + Object.defineProperty(SomeDirective.prototype, "none", { + enumerable: true, + configurable: true + }); + Object.defineProperty(SomeDirective.prototype, "incomplete"); + SomeDirective.decorators = [ + { type: Directive, args: [{ selector: '[someDirective]' },] } + ]; + SomeDirective.propDecorators = { + "setter": [{ type: Input },], + "getter": [{ type: Output },], + "setterAndGetter": [{ type: Input },], + }; + return SomeDirective; + }()); + `, +}; const SIMPLE_CLASS_FILE = { name: '/simple_class.js', @@ -627,7 +673,54 @@ describe('Esm5ReflectionHost', () => { const input2 = members.find(member => member.name === 'input2') !; expect(input2.kind).toEqual(ClassMemberKind.Property); expect(input2.isStatic).toEqual(false); - expect(input1.decorators !.map(d => d.name)).toEqual(['Input']); + expect(input2.decorators !.map(d => d.name)).toEqual(['Input']); + }); + + it('should find Object.defineProperty members on a class', () => { + const program = makeTestProgram(ACCESSORS_FILE); + const host = new Esm5ReflectionHost(false, program.getTypeChecker()); + const classNode = + getDeclaration(program, ACCESSORS_FILE.name, 'SomeDirective', ts.isVariableDeclaration); + const members = host.getMembersOfClass(classNode); + + const setter = members.find(member => member.name === 'setter') !; + expect(setter.kind).toEqual(ClassMemberKind.Setter); + expect(setter.isStatic).toEqual(false); + expect(setter.value).toBeNull(); + expect(setter.decorators !.map(d => d.name)).toEqual(['Input']); + expect(ts.isFunctionExpression(setter.implementation !)).toEqual(true); + expect((setter.implementation as ts.FunctionExpression).body.statements[0].getText()) + .toEqual('this.value = value;'); + + const getter = members.find(member => member.name === 'getter') !; + expect(getter.kind).toEqual(ClassMemberKind.Getter); + expect(getter.isStatic).toEqual(false); + expect(getter.value).toBeNull(); + expect(getter.decorators !.map(d => d.name)).toEqual(['Output']); + expect(ts.isFunctionExpression(getter.implementation !)).toEqual(true); + expect((getter.implementation as ts.FunctionExpression).body.statements[0].getText()) + .toEqual('return null;'); + + const [combinedSetter, combinedGetter] = + members.filter(member => member.name === 'setterAndGetter'); + expect(combinedSetter.kind).toEqual(ClassMemberKind.Setter); + expect(combinedSetter.isStatic).toEqual(false); + expect(combinedSetter.decorators !.map(d => d.name)).toEqual(['Input']); + expect(combinedGetter.kind).toEqual(ClassMemberKind.Getter); + expect(combinedGetter.isStatic).toEqual(false); + expect(combinedGetter.decorators !.map(d => d.name)).toEqual([]); + + const staticSetter = members.find(member => member.name === 'staticSetter') !; + expect(staticSetter.kind).toEqual(ClassMemberKind.Setter); + expect(staticSetter.isStatic).toEqual(true); + expect(staticSetter.value).toBeNull(); + expect(staticSetter.decorators !.map(d => d.name)).toEqual([]); + + const none = members.find(member => member.name === 'none'); + expect(none).toBeUndefined(); + + const incomplete = members.find(member => member.name === 'incomplete'); + expect(incomplete).toBeUndefined(); }); it('should find non decorated properties on a class', () => {