From 704775168d7c0cced9aff3da2b2b733b53e9fb35 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Wed, 30 Oct 2019 15:50:19 +0200 Subject: [PATCH] fix(ngcc): generate correct metadata for classes with getter/setter properties (#33514) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While processing class metadata, ngtsc generates a `setClassMetadata()` call which (among other things) contains info about property decorators. Previously, processing getter/setter pairs with some of ngcc's `ReflectionHost`s resulted in multiple metadata entries for the same property, which resulted in duplicate object keys, which in turn causes an error in ES5 strict mode. This commit fixes it by ensuring that there are no duplicate property names in the `setClassMetadata()` calls. In addition, `generateSetClassMetadataCall()` is updated to treat `ClassMember#decorators: []` the same as `ClassMember.decorators: null` (i.e. omitting the `ClassMember` from the generated `setClassMetadata()` call). Alternatively, ngcc's `ReflectionHost`s could be updated to do this transformation (`decorators: []` --> `decorators: null`) when reflecting on class members, but this would require changes in many places and be less future-proof. For example, given a class such as: ```ts class Foo { @Input() get bar() { return 'bar'; } set bar(value: any) {} } ``` ...previously the generated `setClassMetadata()` call would look like: ```ts ɵsetClassMetadata(..., { bar: [{type: Input}], bar: [], }); ``` The same class will now result in a call like: ```ts ɵsetClassMetadata(..., { bar: [{type: Input}], }); ``` Fixes #30569 PR Close #33514 --- .../ngcc/test/integration/ngcc_spec.ts | 35 +++++++++++++++++++ .../ngcc/test/rendering/renderer_spec.ts | 2 +- .../src/ngtsc/annotations/src/metadata.ts | 16 +++++++-- .../ngtsc/annotations/test/metadata_spec.ts | 31 ++++++++++++---- 4 files changed, 74 insertions(+), 10 deletions(-) diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index 9e51fa0cf1..c5676b6def 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -112,6 +112,41 @@ runInEachFileSystem(() => { expect(loadPackage('local-package', _('/dist')).__processed_by_ivy_ngcc__).toBeUndefined(); }); + it('should generate correct metadata for decorated getter/setter properties', () => { + genNodeModules({ + 'test-package': { + '/index.ts': ` + import {Directive, Input, NgModule} from '@angular/core'; + + @Directive({selector: '[foo]'}) + export class FooDirective { + @Input() get bar() { return 'bar'; } + set bar(value: string) {} + } + + @NgModule({ + declarations: [FooDirective], + }) + export class FooModule {} + `, + }, + }); + + mainNgcc({ + basePath: '/node_modules', + targetEntryPointPath: 'test-package', + propertiesToConsider: ['main'], + }); + + const jsContents = fs.readFile(_(`/node_modules/test-package/index.js`)).replace(/\s+/g, ' '); + expect(jsContents) + .toContain( + '/*@__PURE__*/ ɵngcc0.ɵsetClassMetadata(FooDirective, ' + + '[{ type: Directive, args: [{ selector: \'[foo]\' }] }], ' + + 'function () { return []; }, ' + + '{ bar: [{ type: Input }] });'); + }); + describe('in async mode', () => { it('should run ngcc without errors for fesm2015', async() => { const promise = mainNgcc({ diff --git a/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts b/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts index 732870f04a..49771e9863 100644 --- a/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts @@ -264,7 +264,7 @@ A.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: A, selectors: [["", "a", ""]] });` .toEqual(`/*@__PURE__*/ ɵngcc0.ɵsetClassMetadata(A, [{ type: Directive, args: [{ selector: '[a]' }] - }], null, { foo: [] });`); + }], null, null);`); }); it('should call removeDecorators with the source code, a map of class decorators that have been analyzed', diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts b/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts index 268e842b56..9338c056de 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts @@ -56,10 +56,20 @@ export function generateSetClassMetadataCall( // Do the same for property decorators. let metaPropDecorators: ts.Expression = ts.createNull(); + const classMembers = reflection.getMembersOfClass(clazz).filter( + member => !member.isStatic && member.decorators !== null && member.decorators.length > 0); + const duplicateDecoratedMemberNames = + classMembers.map(member => member.name).filter((name, i, arr) => arr.indexOf(name) < i); + if (duplicateDecoratedMemberNames.length > 0) { + // This should theoretically never happen, because the only way to have duplicate instance + // member names is getter/setter pairs and decorators cannot appear in both a getter and the + // corresponding setter. + throw new Error( + `Duplicate decorated properties found on class '${clazz.name.text}': ` + + duplicateDecoratedMemberNames.join(', ')); + } const decoratedMembers = - reflection.getMembersOfClass(clazz) - .filter(member => !member.isStatic && member.decorators !== null) - .map(member => classMemberToMetadata(member.name, member.decorators !, isCore)); + classMembers.map(member => classMemberToMetadata(member.name, member.decorators !, isCore)); if (decoratedMembers.length > 0) { metaPropDecorators = ts.createObjectLiteral(decoratedMembers); } diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/metadata_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/metadata_spec.ts index de27ae47be..6697ec5776 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/metadata_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/metadata_spec.ts @@ -64,6 +64,23 @@ runInEachFileSystem(() => { expect(res).toContain(`{ foo: [{ type: Input }], bar: [{ type: Input, args: ['value'] }] })`); }); + it('should convert decorated field getter/setter metadata', () => { + const res = compileAndPrint(` + import {Component, Input} from '@angular/core'; + + @Component('metadata') class Target { + @Input() get foo() { return this._foo; } + set foo(value: string) { this._foo = value; } + private _foo: string; + + get bar() { return this._bar; } + @Input('value') set bar(value: string) { this._bar = value; } + private _bar: string; + } + `); + expect(res).toContain(`{ foo: [{ type: Input }], bar: [{ type: Input, args: ['value'] }] })`); + }); + it('should not convert non-angular decorators to metadata', () => { const res = compileAndPrint(` declare function NotAComponent(...args: any[]): any; @@ -86,12 +103,14 @@ runInEachFileSystem(() => { ` }; - const {program} = makeProgram([ - CORE, { - name: _('/index.ts'), - contents, - } - ]); + const {program} = makeProgram( + [ + CORE, { + name: _('/index.ts'), + contents, + } + ], + {target: ts.ScriptTarget.ES2015}); const host = new TypeScriptReflectionHost(program.getTypeChecker()); const target = getDeclaration(program, _('/index.ts'), 'Target', ts.isClassDeclaration); const call = generateSetClassMetadataCall(target, host, NOOP_DEFAULT_IMPORT_RECORDER, false);