Skip to content

Commit 21bd8c9

Browse files
gkalpakkara
authored andcommitted
fix(ngcc): generate correct metadata for classes with getter/setter properties (#33514)
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
1 parent 1c1240c commit 21bd8c9

File tree

4 files changed

+74
-10
lines changed

4 files changed

+74
-10
lines changed

packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,41 @@ runInEachFileSystem(() => {
112112
expect(loadPackage('local-package', _('/dist')).__processed_by_ivy_ngcc__).toBeUndefined();
113113
});
114114

115+
it('should generate correct metadata for decorated getter/setter properties', () => {
116+
genNodeModules({
117+
'test-package': {
118+
'/index.ts': `
119+
import {Directive, Input, NgModule} from '@angular/core';
120+
121+
@Directive({selector: '[foo]'})
122+
export class FooDirective {
123+
@Input() get bar() { return 'bar'; }
124+
set bar(value: string) {}
125+
}
126+
127+
@NgModule({
128+
declarations: [FooDirective],
129+
})
130+
export class FooModule {}
131+
`,
132+
},
133+
});
134+
135+
mainNgcc({
136+
basePath: '/node_modules',
137+
targetEntryPointPath: 'test-package',
138+
propertiesToConsider: ['main'],
139+
});
140+
141+
const jsContents = fs.readFile(_(`/node_modules/test-package/index.js`)).replace(/\s+/g, ' ');
142+
expect(jsContents)
143+
.toContain(
144+
'/*@__PURE__*/ ɵngcc0.ɵsetClassMetadata(FooDirective, ' +
145+
'[{ type: Directive, args: [{ selector: \'[foo]\' }] }], ' +
146+
'function () { return []; }, ' +
147+
'{ bar: [{ type: Input }] });');
148+
});
149+
115150
describe('in async mode', () => {
116151
it('should run ngcc without errors for fesm2015', async() => {
117152
const promise = mainNgcc({

packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ A.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: A, selectors: [["", "a", ""]] });`
264264
.toEqual(`/*@__PURE__*/ ɵngcc0.ɵsetClassMetadata(A, [{
265265
type: Directive,
266266
args: [{ selector: '[a]' }]
267-
}], null, { foo: [] });`);
267+
}], null, null);`);
268268
});
269269

270270
it('should call removeDecorators with the source code, a map of class decorators that have been analyzed',

packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,10 +56,20 @@ export function generateSetClassMetadataCall(
5656

5757
// Do the same for property decorators.
5858
let metaPropDecorators: ts.Expression = ts.createNull();
59+
const classMembers = reflection.getMembersOfClass(clazz).filter(
60+
member => !member.isStatic && member.decorators !== null && member.decorators.length > 0);
61+
const duplicateDecoratedMemberNames =
62+
classMembers.map(member => member.name).filter((name, i, arr) => arr.indexOf(name) < i);
63+
if (duplicateDecoratedMemberNames.length > 0) {
64+
// This should theoretically never happen, because the only way to have duplicate instance
65+
// member names is getter/setter pairs and decorators cannot appear in both a getter and the
66+
// corresponding setter.
67+
throw new Error(
68+
`Duplicate decorated properties found on class '${clazz.name.text}': ` +
69+
duplicateDecoratedMemberNames.join(', '));
70+
}
5971
const decoratedMembers =
60-
reflection.getMembersOfClass(clazz)
61-
.filter(member => !member.isStatic && member.decorators !== null)
62-
.map(member => classMemberToMetadata(member.name, member.decorators !, isCore));
72+
classMembers.map(member => classMemberToMetadata(member.name, member.decorators !, isCore));
6373
if (decoratedMembers.length > 0) {
6474
metaPropDecorators = ts.createObjectLiteral(decoratedMembers);
6575
}

packages/compiler-cli/src/ngtsc/annotations/test/metadata_spec.ts

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,23 @@ runInEachFileSystem(() => {
6464
expect(res).toContain(`{ foo: [{ type: Input }], bar: [{ type: Input, args: ['value'] }] })`);
6565
});
6666

67+
it('should convert decorated field getter/setter metadata', () => {
68+
const res = compileAndPrint(`
69+
import {Component, Input} from '@angular/core';
70+
71+
@Component('metadata') class Target {
72+
@Input() get foo() { return this._foo; }
73+
set foo(value: string) { this._foo = value; }
74+
private _foo: string;
75+
76+
get bar() { return this._bar; }
77+
@Input('value') set bar(value: string) { this._bar = value; }
78+
private _bar: string;
79+
}
80+
`);
81+
expect(res).toContain(`{ foo: [{ type: Input }], bar: [{ type: Input, args: ['value'] }] })`);
82+
});
83+
6784
it('should not convert non-angular decorators to metadata', () => {
6885
const res = compileAndPrint(`
6986
declare function NotAComponent(...args: any[]): any;
@@ -86,12 +103,14 @@ runInEachFileSystem(() => {
86103
`
87104
};
88105

89-
const {program} = makeProgram([
90-
CORE, {
91-
name: _('/index.ts'),
92-
contents,
93-
}
94-
]);
106+
const {program} = makeProgram(
107+
[
108+
CORE, {
109+
name: _('/index.ts'),
110+
contents,
111+
}
112+
],
113+
{target: ts.ScriptTarget.ES2015});
95114
const host = new TypeScriptReflectionHost(program.getTypeChecker());
96115
const target = getDeclaration(program, _('/index.ts'), 'Target', ts.isClassDeclaration);
97116
const call = generateSetClassMetadataCall(target, host, NOOP_DEFAULT_IMPORT_RECORDER, false);

0 commit comments

Comments
 (0)