Skip to content

Commit fc6f481

Browse files
JoostKAndrewKushnir
authored andcommitted
fix(ivy): ngcc - render decorators in UMD and CommonJS bundles correctly (angular#31614)
In angular#31426 a fix was implemented to render namespaced decorator imports correctly, however it turns out that the fix only worked when decorator information was extracted from static properties, not when using `__decorate` calls. This commit fixes the issue by creating the decorator metadata with the full decorator expression, instead of only its name. Closes angular#31394 PR Close angular#31614
1 parent 80f290e commit fc6f481

File tree

6 files changed

+146
-2
lines changed

6 files changed

+146
-2
lines changed

integration/ngcc/test.sh

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,15 @@ if [[ $? != 0 ]]; then exit 1; fi
6767
grep "_MatMenuBase.ngBaseDef = ɵngcc0.ɵɵdefineBase({ inputs: {" node_modules/@angular/material/esm5/menu.es5.js
6868
if [[ $? != 0 ]]; then exit 1; fi
6969

70-
# Did it handle namespace imported decorators in UMD?
70+
# Did it handle namespace imported decorators in UMD using `__decorate` syntax?
71+
grep "type: core.Injectable" node_modules/@angular/common/bundles/common.umd.js
72+
# (and ensure the @angular/common package is indeed using `__decorate` syntax)
73+
grep "JsonPipe = __decorate(" node_modules/@angular/common/bundles/common.umd.js.__ivy_ngcc_bak
74+
75+
# Did it handle namespace imported decorators in UMD using static properties?
7176
grep "type: core.Injectable," node_modules/@angular/cdk/bundles/cdk-a11y.umd.js
77+
# (and ensure the @angular/cdk/a11y package is indeed using static properties)
78+
grep "FocusMonitor.decorators =" node_modules/@angular/cdk/bundles/cdk-a11y.umd.js.__ivy_ngcc_bak
7279

7380
# Can it be safely run again (as a noop)?
7481
# And check that it logged skipping compilation as expected

packages/compiler-cli/ngcc/src/host/esm2015_host.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -957,7 +957,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
957957

958958
return {
959959
name: decoratorIdentifier.text,
960-
identifier: decoratorIdentifier,
960+
identifier: decoratorExpression,
961961
import: this.getImportOfIdentifier(decoratorIdentifier),
962962
node: call,
963963
args: Array.from(call.arguments),

packages/compiler-cli/ngcc/test/host/commonjs_host_import_helper_spec.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ exports.OtherDirective = OtherDirective;
8686

8787
const decorator = decorators[0];
8888
expect(decorator.name).toEqual('Directive');
89+
expect(decorator.identifier.getText()).toEqual('core.Directive');
8990
expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'});
9091
expect(decorator.args !.map(arg => arg.getText())).toEqual([
9192
'{ selector: \'[someDirective]\' }',

packages/compiler-cli/ngcc/test/host/esm2015_host_import_helper_spec.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ runInEachFileSystem(() => {
145145

146146
const decorator = decorators[0];
147147
expect(decorator.name).toEqual('Directive');
148+
expect(decorator.identifier.getText()).toEqual('Directive');
148149
expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'});
149150
expect(decorator.args !.map(arg => arg.getText())).toEqual([
150151
'{ selector: \'[someDirective]\' }',
@@ -166,6 +167,7 @@ runInEachFileSystem(() => {
166167

167168
const decorator = decorators[0];
168169
expect(decorator.name).toEqual('Directive');
170+
expect(decorator.identifier.getText()).toEqual('Directive');
169171
expect(decorator.import).toEqual({name: 'Directive', from: './directives'});
170172
expect(decorator.args !.map(arg => arg.getText())).toEqual([
171173
'{ selector: \'[someDirective]\' }',

packages/compiler-cli/ngcc/test/host/esm5_host_import_helper_spec.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ export { SomeDirective };
165165

166166
const decorator = decorators[0];
167167
expect(decorator.name).toEqual('Directive');
168+
expect(decorator.identifier.getText()).toEqual('Directive');
168169
expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'});
169170
expect(decorator.args !.map(arg => arg.getText())).toEqual([
170171
'{ selector: \'[someDirective]\' }',
@@ -185,6 +186,7 @@ export { SomeDirective };
185186

186187
const decorator = decorators[0];
187188
expect(decorator.name).toEqual('Directive');
189+
expect(decorator.identifier.getText()).toEqual('Directive');
188190
expect(decorator.import).toEqual({name: 'Directive', from: './directives'});
189191
expect(decorator.args !.map(arg => arg.getText())).toEqual([
190192
'{ selector: \'[someDirective]\' }',
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
/**
2+
* @license
3+
* Copyright Google Inc. All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {absoluteFrom} from '../../../src/ngtsc/file_system';
10+
import {TestFile, runInEachFileSystem} from '../../../src/ngtsc/file_system/testing';
11+
import {ClassMemberKind, isNamedVariableDeclaration} from '../../../src/ngtsc/reflection';
12+
import {getDeclaration} from '../../../src/ngtsc/testing';
13+
import {loadTestFiles} from '../../../test/helpers';
14+
import {UmdReflectionHost} from '../../src/host/umd_host';
15+
import {MockLogger} from '../helpers/mock_logger';
16+
import {makeTestBundleProgram} from '../helpers/utils';
17+
18+
import {expectTypeValueReferencesForParameters} from './util';
19+
20+
runInEachFileSystem(() => {
21+
describe('UmdReflectionHost [import helper style]', () => {
22+
let _: typeof absoluteFrom;
23+
24+
let SOME_DIRECTIVE_FILE: TestFile;
25+
26+
beforeEach(() => {
27+
_ = absoluteFrom;
28+
29+
SOME_DIRECTIVE_FILE = {
30+
name: _('/some_directive.umd.js'),
31+
contents: `
32+
(function (global, factory) {
33+
typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('@angular/core')) :
34+
typeof define === 'function' && define.amd ? define('some_directive', ['exports', '@angular/core'], factory) :
35+
(factory(global.some_directive,global.ng.core));
36+
}(this, (function (exports,core) { 'use strict';
37+
38+
var __decorate = null;
39+
var __metadata = null;
40+
var __param = null;
41+
42+
var INJECTED_TOKEN = new InjectionToken('injected');
43+
var ViewContainerRef = {};
44+
var TemplateRef = {};
45+
46+
var SomeDirective = (function() {
47+
function SomeDirective(_viewContainer, _template, injected) {}
48+
__decorate([
49+
core.Input(),
50+
__metadata("design:type", String)
51+
], SomeDirective.prototype, "input1", void 0);
52+
__decorate([
53+
core.Input(),
54+
__metadata("design:type", Number)
55+
], SomeDirective.prototype, "input2", void 0);
56+
SomeDirective = __decorate([
57+
core.Directive({ selector: '[someDirective]' }),
58+
__param(2, core.Inject(INJECTED_TOKEN)),
59+
__metadata("design:paramtypes", [ViewContainerRef, TemplateRef, String])
60+
], SomeDirective);
61+
return SomeDirective;
62+
}());
63+
exports.SomeDirective = SomeDirective;
64+
})));`,
65+
};
66+
});
67+
68+
describe('getDecoratorsOfDeclaration()', () => {
69+
it('should find the decorators on a class', () => {
70+
loadTestFiles([SOME_DIRECTIVE_FILE]);
71+
const {program, host: compilerHost} = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name);
72+
const host = new UmdReflectionHost(new MockLogger(), false, program, compilerHost);
73+
const classNode = getDeclaration(
74+
program, SOME_DIRECTIVE_FILE.name, 'SomeDirective', isNamedVariableDeclaration);
75+
const decorators = host.getDecoratorsOfDeclaration(classNode) !;
76+
77+
expect(decorators).toBeDefined();
78+
expect(decorators.length).toEqual(1);
79+
80+
const decorator = decorators[0];
81+
expect(decorator.name).toEqual('Directive');
82+
expect(decorator.identifier.getText()).toEqual('core.Directive');
83+
expect(decorator.import).toEqual({name: 'Directive', from: '@angular/core'});
84+
expect(decorator.args !.map(arg => arg.getText())).toEqual([
85+
'{ selector: \'[someDirective]\' }',
86+
]);
87+
});
88+
});
89+
90+
describe('getMembersOfClass()', () => {
91+
it('should find decorated members on a class', () => {
92+
loadTestFiles([SOME_DIRECTIVE_FILE]);
93+
const {program, host: compilerHost} = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name);
94+
const host = new UmdReflectionHost(new MockLogger(), false, program, compilerHost);
95+
const classNode = getDeclaration(
96+
program, SOME_DIRECTIVE_FILE.name, 'SomeDirective', isNamedVariableDeclaration);
97+
const members = host.getMembersOfClass(classNode);
98+
99+
const input1 = members.find(member => member.name === 'input1') !;
100+
expect(input1.kind).toEqual(ClassMemberKind.Property);
101+
expect(input1.isStatic).toEqual(false);
102+
expect(input1.decorators !.map(d => d.name)).toEqual(['Input']);
103+
104+
const input2 = members.find(member => member.name === 'input2') !;
105+
expect(input2.kind).toEqual(ClassMemberKind.Property);
106+
expect(input2.isStatic).toEqual(false);
107+
expect(input1.decorators !.map(d => d.name)).toEqual(['Input']);
108+
});
109+
110+
describe('getConstructorParameters', () => {
111+
it('should find the decorated constructor parameters', () => {
112+
loadTestFiles([SOME_DIRECTIVE_FILE]);
113+
const {program, host: compilerHost} = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name);
114+
const host = new UmdReflectionHost(new MockLogger(), false, program, compilerHost);
115+
const classNode = getDeclaration(
116+
program, SOME_DIRECTIVE_FILE.name, 'SomeDirective', isNamedVariableDeclaration);
117+
const parameters = host.getConstructorParameters(classNode);
118+
119+
expect(parameters).toBeDefined();
120+
expect(parameters !.map(parameter => parameter.name)).toEqual([
121+
'_viewContainer', '_template', 'injected'
122+
]);
123+
expectTypeValueReferencesForParameters(parameters !, [
124+
'ViewContainerRef',
125+
'TemplateRef',
126+
null,
127+
]);
128+
});
129+
});
130+
});
131+
});
132+
});

0 commit comments

Comments
 (0)