Skip to content

Commit 07d3d8c

Browse files
clydinclydin
committed
fix(@angular-devkit/build-angular): correctly wrap CommonJS exported enums when optimizing
When optimizing a CommonJS exported enum, the build optimizer enum wrapping pass was previously dropping the `exports` object assignment from the enum wrapper function call expression. This would not occur with application code but is possible with library code that was built with TypeScript and shipped as CommonJS. Assuming the following TypeScript enum: ``` export enum ChangeDetectionStrategy { OnPush = 0, Default = 1, } ``` TypeScript 5.1 will emit an exported enum for CommonJS as follows: ``` exports.ChangeDetectionStrategy = void 0; var ChangeDetectionStrategy; (function (ChangeDetectionStrategy) { ChangeDetectionStrategy[ChangeDetectionStrategy["OnPush"] = 0] = "OnPush"; ChangeDetectionStrategy[ChangeDetectionStrategy["Default"] = 1] = "Default"; })(ChangeDetectionStrategy || (exports.ChangeDetectionStrategy = ChangeDetectionStrategy = {})); ``` The build optimizer would previously transform this into: ``` exports.ChangeDetectionStrategy = void 0; var ChangeDetectionStrategy = /*#__PURE__*/ (() => { ChangeDetectionStrategy = ChangeDetectionStrategy || {}; ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 5)] = "OnPush"; ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 8)] = "Default"; return ChangeDetectionStrategy; })(); ``` But this has a defect wherein the `exports` assignment is dropped. To rectify this situation, the build optimizer will now transform the code into: ``` exports.ChangeDetectionStrategy = void 0; var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) { ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush"; ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default"; return ChangeDetectionStrategy; })(ChangeDetectionStrategy || (exports.ChangeDetectionStrategy = ChangeDetectionStrategy = {})) ``` This retains the `exports` assignment. (cherry picked from commit 5908ede)
1 parent 7e91d47 commit 07d3d8c

File tree

2 files changed

+94
-82
lines changed

2 files changed

+94
-82
lines changed

‎packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums.ts

Lines changed: 26 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,22 @@ export default function (): PluginObj {
5656
return;
5757
}
5858

59-
const enumCallArgument = nextExpression.node.arguments[0];
60-
if (!types.isLogicalExpression(enumCallArgument, { operator: '||' })) {
59+
const enumCallArgument = nextExpression.get('arguments')[0];
60+
if (!enumCallArgument.isLogicalExpression({ operator: '||' })) {
6161
return;
6262
}
6363

64+
const leftCallArgument = enumCallArgument.get('left');
65+
const rightCallArgument = enumCallArgument.get('right');
66+
6467
// Check if identifiers match var declaration
6568
if (
66-
!types.isIdentifier(enumCallArgument.left) ||
67-
!nextExpression.scope.bindingIdentifierEquals(enumCallArgument.left.name, declarationId)
69+
!leftCallArgument.isIdentifier() ||
70+
!nextExpression.scope.bindingIdentifierEquals(
71+
leftCallArgument.node.name,
72+
declarationId,
73+
) ||
74+
!rightCallArgument.isAssignmentExpression()
6875
) {
6976
return;
7077
}
@@ -74,13 +81,9 @@ export default function (): PluginObj {
7481
return;
7582
}
7683

77-
const enumCalleeParam = enumCallee.node.params[0];
78-
const isEnumCalleeMatching =
79-
types.isIdentifier(enumCalleeParam) && enumCalleeParam.name === declarationId.name;
80-
81-
let enumAssignments: types.ExpressionStatement[] | undefined;
82-
if (isEnumCalleeMatching) {
83-
enumAssignments = [];
84+
const parameterId = enumCallee.get('params')[0];
85+
if (!parameterId.isIdentifier()) {
86+
return;
8487
}
8588

8689
// Check if all enum member values are pure.
@@ -100,56 +103,31 @@ export default function (): PluginObj {
100103
}
101104

102105
hasElements = true;
103-
enumAssignments?.push(enumStatement.node);
104106
}
105107

106108
// If there are no enum elements then there is nothing to wrap
107109
if (!hasElements) {
108110
return;
109111
}
110112

113+
// Update right-side of initializer call argument to remove redundant assignment
114+
if (rightCallArgument.get('left').isIdentifier()) {
115+
rightCallArgument.replaceWith(rightCallArgument.get('right'));
116+
}
117+
118+
// Add a return statement to the enum initializer block
119+
enumCallee
120+
.get('body')
121+
.node.body.push(types.returnStatement(types.cloneNode(parameterId.node)));
122+
111123
// Remove existing enum initializer
112124
const enumInitializer = nextExpression.node;
113125
nextExpression.remove();
114126

115-
// Create IIFE block contents
116-
let blockContents;
117-
if (enumAssignments) {
118-
// Loose mode
119-
blockContents = [
120-
types.expressionStatement(
121-
types.assignmentExpression(
122-
'=',
123-
types.cloneNode(declarationId),
124-
types.logicalExpression(
125-
'||',
126-
types.cloneNode(declarationId),
127-
types.objectExpression([]),
128-
),
129-
),
130-
),
131-
...enumAssignments,
132-
];
133-
} else {
134-
blockContents = [types.expressionStatement(enumInitializer)];
135-
}
136-
137-
// Wrap existing enum initializer in a pure annotated IIFE
138-
const container = types.arrowFunctionExpression(
139-
[],
140-
types.blockStatement([
141-
...blockContents,
142-
types.returnStatement(types.cloneNode(declarationId)),
143-
]),
144-
);
145-
const replacementInitializer = types.callExpression(
146-
types.parenthesizedExpression(container),
147-
[],
148-
);
149-
annotateAsPure(replacementInitializer);
127+
annotateAsPure(enumInitializer);
150128

151129
// Add the wrapped enum initializer directly to the variable declaration
152-
declaration.get('init').replaceWith(replacementInitializer);
130+
declaration.get('init').replaceWith(enumInitializer);
153131
},
154132
},
155133
};

‎packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums_spec.ts

Lines changed: 68 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,15 @@ function testCaseNoChange(input: string): void {
3131
testCase({ input, expected: input });
3232
}
3333

34+
// The majority of these test cases are based off TypeScript emitted enum code for the FW's
35+
// `ChangedDetectionStrategy` enum.
36+
// https://github.com/angular/angular/blob/55d412c5b1b0ba9b03174f7ad9907961fcafa970/packages/core/src/change_detection/constants.ts#L18
37+
// ```
38+
// export enum ChangeDetectionStrategy {
39+
// OnPush = 0,
40+
// Default = 1,
41+
// }
42+
// ```
3443
describe('adjust-typescript-enums Babel plugin', () => {
3544
it('wraps unexported TypeScript enums', () => {
3645
testCase({
@@ -42,12 +51,11 @@ describe('adjust-typescript-enums Babel plugin', () => {
4251
})(ChangeDetectionStrategy || (ChangeDetectionStrategy = {}));
4352
`,
4453
expected: `
45-
var ChangeDetectionStrategy = /*#__PURE__*/ (() => {
46-
ChangeDetectionStrategy = ChangeDetectionStrategy || {};
54+
var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) {
4755
ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush";
4856
ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default";
4957
return ChangeDetectionStrategy;
50-
})();
58+
})(ChangeDetectionStrategy || {});
5159
`,
5260
});
5361
});
@@ -62,12 +70,46 @@ describe('adjust-typescript-enums Babel plugin', () => {
6270
})(ChangeDetectionStrategy || (ChangeDetectionStrategy = {}));
6371
`,
6472
expected: `
65-
export var ChangeDetectionStrategy = /*#__PURE__*/ (() => {
66-
ChangeDetectionStrategy = ChangeDetectionStrategy || {};
73+
export var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) {
6774
ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush";
6875
ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default";
6976
return ChangeDetectionStrategy;
70-
})();
77+
})(ChangeDetectionStrategy || {});
78+
`,
79+
});
80+
});
81+
82+
// Even with recent improvements this case is and was never wrapped. However, it also was not broken
83+
// by the transformation. This test ensures that this older emitted enum form does not break with
84+
// any future changes. Over time this older form will be encountered less and less frequently.
85+
// In the future this test case could be considered for removal.
86+
it('does not wrap exported TypeScript enums from CommonJS (<5.1)', () => {
87+
testCaseNoChange(
88+
`
89+
var ChangeDetectionStrategy;
90+
(function (ChangeDetectionStrategy) {
91+
ChangeDetectionStrategy[ChangeDetectionStrategy["OnPush"] = 0] = "OnPush";
92+
ChangeDetectionStrategy[ChangeDetectionStrategy["Default"] = 1] = "Default";
93+
})(ChangeDetectionStrategy = exports.ChangeDetectionStrategy || (exports.ChangeDetectionStrategy = {}));
94+
`,
95+
);
96+
});
97+
98+
it('wraps exported TypeScript enums from CommonJS (5.1+)', () => {
99+
testCase({
100+
input: `
101+
var ChangeDetectionStrategy;
102+
(function (ChangeDetectionStrategy) {
103+
ChangeDetectionStrategy[ChangeDetectionStrategy["OnPush"] = 0] = "OnPush";
104+
ChangeDetectionStrategy[ChangeDetectionStrategy["Default"] = 1] = "Default";
105+
})(ChangeDetectionStrategy || (exports.ChangeDetectionStrategy = ChangeDetectionStrategy = {}));
106+
`,
107+
expected: `
108+
var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) {
109+
ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush";
110+
ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default";
111+
return ChangeDetectionStrategy;
112+
})(ChangeDetectionStrategy || (exports.ChangeDetectionStrategy = ChangeDetectionStrategy = {}));
71113
`,
72114
});
73115
});
@@ -82,12 +124,11 @@ describe('adjust-typescript-enums Babel plugin', () => {
82124
})(ChangeDetectionStrategy || (ChangeDetectionStrategy = {}));
83125
`,
84126
expected: `
85-
export var ChangeDetectionStrategy = /*#__PURE__*/ (() => {
86-
ChangeDetectionStrategy = ChangeDetectionStrategy || {};
127+
export var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) {
87128
ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 5)] = "OnPush";
88129
ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 8)] = "Default";
89130
return ChangeDetectionStrategy;
90-
})();
131+
})(ChangeDetectionStrategy || {});
91132
`,
92133
});
93134
});
@@ -103,13 +144,12 @@ describe('adjust-typescript-enums Babel plugin', () => {
103144
})(NotificationKind || (NotificationKind = {}));
104145
`,
105146
expected: `
106-
var NotificationKind = /*#__PURE__*/ (() => {
107-
NotificationKind = NotificationKind || {};
147+
var NotificationKind = /*#__PURE__*/ (function (NotificationKind) {
108148
NotificationKind["NEXT"] = "N";
109149
NotificationKind["ERROR"] = "E";
110150
NotificationKind["COMPLETE"] = "C";
111151
return NotificationKind;
112-
})();
152+
})(NotificationKind || {});
113153
`,
114154
});
115155
});
@@ -125,14 +165,12 @@ describe('adjust-typescript-enums Babel plugin', () => {
125165
})(NotificationKind$1 || (NotificationKind$1 = {}));
126166
`,
127167
expected: `
128-
var NotificationKind$1 = /*#__PURE__*/ (() => {
129-
(function (NotificationKind) {
130-
NotificationKind["NEXT"] = "N";
131-
NotificationKind["ERROR"] = "E";
132-
NotificationKind["COMPLETE"] = "C";
133-
})(NotificationKind$1 || (NotificationKind$1 = {}));
134-
return NotificationKind$1;
135-
})();
168+
var NotificationKind$1 = /*#__PURE__*/ (function (NotificationKind) {
169+
NotificationKind["NEXT"] = "N";
170+
NotificationKind["ERROR"] = "E";
171+
NotificationKind["COMPLETE"] = "C";
172+
return NotificationKind;
173+
})(NotificationKind$1 || {});
136174
`,
137175
});
138176
});
@@ -160,8 +198,7 @@ describe('adjust-typescript-enums Babel plugin', () => {
160198
* Supported http methods.
161199
* @deprecated use @angular/common/http instead
162200
*/
163-
var RequestMethod = /*#__PURE__*/ (() => {
164-
RequestMethod = RequestMethod || {};
201+
var RequestMethod = /*#__PURE__*/ (function (RequestMethod) {
165202
RequestMethod[(RequestMethod["Get"] = 0)] = "Get";
166203
RequestMethod[(RequestMethod["Post"] = 1)] = "Post";
167204
RequestMethod[(RequestMethod["Put"] = 2)] = "Put";
@@ -170,7 +207,7 @@ describe('adjust-typescript-enums Babel plugin', () => {
170207
RequestMethod[(RequestMethod["Head"] = 5)] = "Head";
171208
RequestMethod[(RequestMethod["Patch"] = 6)] = "Patch";
172209
return RequestMethod;
173-
})();
210+
})(RequestMethod || {});
174211
`,
175212
});
176213
});
@@ -208,17 +245,16 @@ describe('adjust-typescript-enums Babel plugin', () => {
208245
})(ChangeDetectionStrategy || (ChangeDetectionStrategy = {}));
209246
`,
210247
expected: `
211-
var ChangeDetectionStrategy = /*#__PURE__*/ (() => {
212-
ChangeDetectionStrategy = ChangeDetectionStrategy || {};
248+
var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) {
213249
ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush";
214250
ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default";
215251
return ChangeDetectionStrategy;
216-
})();
252+
})(ChangeDetectionStrategy || {});
217253
`,
218254
});
219255
});
220256

221-
it('should not wrap TypeScript enums if the declaration identifier has been renamed to avoid collisions', () => {
257+
it('should wrap TypeScript enums if the declaration identifier has been renamed to avoid collisions', () => {
222258
testCase({
223259
input: `
224260
var ChangeDetectionStrategy$1;
@@ -228,13 +264,11 @@ describe('adjust-typescript-enums Babel plugin', () => {
228264
})(ChangeDetectionStrategy$1 || (ChangeDetectionStrategy$1 = {}));
229265
`,
230266
expected: `
231-
var ChangeDetectionStrategy$1 = /*#__PURE__*/ (() => {
232-
(function (ChangeDetectionStrategy) {
233-
ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush";
234-
ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default";
235-
})(ChangeDetectionStrategy$1 || (ChangeDetectionStrategy$1 = {}));
236-
return ChangeDetectionStrategy$1;
237-
})();
267+
var ChangeDetectionStrategy$1 = /*#__PURE__*/ (function (ChangeDetectionStrategy) {
268+
ChangeDetectionStrategy[ChangeDetectionStrategy["OnPush"] = 0] = "OnPush";
269+
ChangeDetectionStrategy[ChangeDetectionStrategy["Default"] = 1] = "Default";
270+
return ChangeDetectionStrategy;
271+
})(ChangeDetectionStrategy$1 || {});
238272
`,
239273
});
240274
});

0 commit comments

Comments
 (0)