From 5908ede3d676f2b87f68bc976bd741b7725ab1c3 Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Thu, 13 Jul 2023 15:32:36 -0400 Subject: [PATCH] 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. --- .../babel/plugins/adjust-typescript-enums.ts | 74 +++++------- .../plugins/adjust-typescript-enums_spec.ts | 105 ++++++++++++------ 2 files changed, 97 insertions(+), 82 deletions(-) diff --git a/packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums.ts b/packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums.ts index 532f178c0d7b..0554e738fa46 100644 --- a/packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums.ts +++ b/packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums.ts @@ -56,15 +56,22 @@ export default function (): PluginObj { return; } - const enumCallArgument = nextExpression.node.arguments[0]; - if (!types.isLogicalExpression(enumCallArgument, { operator: '||' })) { + const enumCallArgument = nextExpression.get('arguments')[0]; + if (!enumCallArgument.isLogicalExpression({ operator: '||' })) { return; } + const leftCallArgument = enumCallArgument.get('left'); + const rightCallArgument = enumCallArgument.get('right'); + // Check if identifiers match var declaration if ( - !types.isIdentifier(enumCallArgument.left) || - !nextExpression.scope.bindingIdentifierEquals(enumCallArgument.left.name, declarationId) + !leftCallArgument.isIdentifier() || + !nextExpression.scope.bindingIdentifierEquals( + leftCallArgument.node.name, + declarationId, + ) || + !rightCallArgument.isAssignmentExpression() ) { return; } @@ -74,13 +81,9 @@ export default function (): PluginObj { return; } - const enumCalleeParam = enumCallee.node.params[0]; - const isEnumCalleeMatching = - types.isIdentifier(enumCalleeParam) && enumCalleeParam.name === declarationId.name; - - let enumAssignments: types.ExpressionStatement[] | undefined; - if (isEnumCalleeMatching) { - enumAssignments = []; + const parameterId = enumCallee.get('params')[0]; + if (!parameterId.isIdentifier()) { + return; } // Check if all enum member values are pure. @@ -100,7 +103,6 @@ export default function (): PluginObj { } hasElements = true; - enumAssignments?.push(enumStatement.node); } // If there are no enum elements then there is nothing to wrap @@ -108,48 +110,24 @@ export default function (): PluginObj { return; } + // Update right-side of initializer call argument to remove redundant assignment + if (rightCallArgument.get('left').isIdentifier()) { + rightCallArgument.replaceWith(rightCallArgument.get('right')); + } + + // Add a return statement to the enum initializer block + enumCallee + .get('body') + .node.body.push(types.returnStatement(types.cloneNode(parameterId.node))); + // Remove existing enum initializer const enumInitializer = nextExpression.node; nextExpression.remove(); - // Create IIFE block contents - let blockContents; - if (enumAssignments) { - // Loose mode - blockContents = [ - types.expressionStatement( - types.assignmentExpression( - '=', - types.cloneNode(declarationId), - types.logicalExpression( - '||', - types.cloneNode(declarationId), - types.objectExpression([]), - ), - ), - ), - ...enumAssignments, - ]; - } else { - blockContents = [types.expressionStatement(enumInitializer)]; - } - - // Wrap existing enum initializer in a pure annotated IIFE - const container = types.arrowFunctionExpression( - [], - types.blockStatement([ - ...blockContents, - types.returnStatement(types.cloneNode(declarationId)), - ]), - ); - const replacementInitializer = types.callExpression( - types.parenthesizedExpression(container), - [], - ); - annotateAsPure(replacementInitializer); + annotateAsPure(enumInitializer); // Add the wrapped enum initializer directly to the variable declaration - declaration.get('init').replaceWith(replacementInitializer); + declaration.get('init').replaceWith(enumInitializer); }, }, }; diff --git a/packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums_spec.ts b/packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums_spec.ts index d5535b74a331..7da222ec016a 100644 --- a/packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums_spec.ts +++ b/packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums_spec.ts @@ -36,6 +36,15 @@ function testCase({ }; } +// The majority of these test cases are based off TypeScript emitted enum code for the FW's +// `ChangedDetectionStrategy` enum. +// https://github.com/angular/angular/blob/55d412c5b1b0ba9b03174f7ad9907961fcafa970/packages/core/src/change_detection/constants.ts#L18 +// ``` +// export enum ChangeDetectionStrategy { +// OnPush = 0, +// Default = 1, +// } +// ``` describe('adjust-typescript-enums Babel plugin', () => { it( 'wraps unexported TypeScript enums', @@ -48,12 +57,11 @@ describe('adjust-typescript-enums Babel plugin', () => { })(ChangeDetectionStrategy || (ChangeDetectionStrategy = {})); `, expected: ` - var ChangeDetectionStrategy = /*#__PURE__*/ (() => { - ChangeDetectionStrategy = ChangeDetectionStrategy || {}; + var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) { ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush"; ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default"; return ChangeDetectionStrategy; - })(); + })(ChangeDetectionStrategy || {}); `, }), ); @@ -69,12 +77,49 @@ describe('adjust-typescript-enums Babel plugin', () => { })(ChangeDetectionStrategy || (ChangeDetectionStrategy = {})); `, expected: ` - export var ChangeDetectionStrategy = /*#__PURE__*/ (() => { - ChangeDetectionStrategy = ChangeDetectionStrategy || {}; + export var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) { ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush"; ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default"; return ChangeDetectionStrategy; - })(); + })(ChangeDetectionStrategy || {}); + `, + }), + ); + + // Even with recent improvements this case is and was never wrapped. However, it also was not broken + // by the transformation. This test ensures that this older emitted enum form does not break with + // any future changes. Over time this older form will be encountered less and less frequently. + // In the future this test case could be considered for removal. + it( + 'does not wrap exported TypeScript enums from CommonJS (<5.1)', + testCase({ + input: ` + var ChangeDetectionStrategy; + (function (ChangeDetectionStrategy) { + ChangeDetectionStrategy[ChangeDetectionStrategy["OnPush"] = 0] = "OnPush"; + ChangeDetectionStrategy[ChangeDetectionStrategy["Default"] = 1] = "Default"; + })(ChangeDetectionStrategy = exports.ChangeDetectionStrategy || (exports.ChangeDetectionStrategy = {})); + `, + expected: NO_CHANGE, + }), + ); + + it( + 'wraps exported TypeScript enums from CommonJS (5.1+)', + testCase({ + input: ` + var ChangeDetectionStrategy; + (function (ChangeDetectionStrategy) { + ChangeDetectionStrategy[ChangeDetectionStrategy["OnPush"] = 0] = "OnPush"; + ChangeDetectionStrategy[ChangeDetectionStrategy["Default"] = 1] = "Default"; + })(ChangeDetectionStrategy || (exports.ChangeDetectionStrategy = ChangeDetectionStrategy = {})); + `, + expected: ` + var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) { + ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush"; + ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default"; + return ChangeDetectionStrategy; + })(ChangeDetectionStrategy || (exports.ChangeDetectionStrategy = ChangeDetectionStrategy = {})); `, }), ); @@ -90,12 +135,11 @@ describe('adjust-typescript-enums Babel plugin', () => { })(ChangeDetectionStrategy || (ChangeDetectionStrategy = {})); `, expected: ` - export var ChangeDetectionStrategy = /*#__PURE__*/ (() => { - ChangeDetectionStrategy = ChangeDetectionStrategy || {}; + export var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) { ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 5)] = "OnPush"; ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 8)] = "Default"; return ChangeDetectionStrategy; - })(); + })(ChangeDetectionStrategy || {}); `, }), ); @@ -112,13 +156,12 @@ describe('adjust-typescript-enums Babel plugin', () => { })(NotificationKind || (NotificationKind = {})); `, expected: ` - var NotificationKind = /*#__PURE__*/ (() => { - NotificationKind = NotificationKind || {}; + var NotificationKind = /*#__PURE__*/ (function (NotificationKind) { NotificationKind["NEXT"] = "N"; NotificationKind["ERROR"] = "E"; NotificationKind["COMPLETE"] = "C"; return NotificationKind; - })(); + })(NotificationKind || {}); `, }), ); @@ -135,14 +178,12 @@ describe('adjust-typescript-enums Babel plugin', () => { })(NotificationKind$1 || (NotificationKind$1 = {})); `, expected: ` - var NotificationKind$1 = /*#__PURE__*/ (() => { - (function (NotificationKind) { - NotificationKind["NEXT"] = "N"; - NotificationKind["ERROR"] = "E"; - NotificationKind["COMPLETE"] = "C"; - })(NotificationKind$1 || (NotificationKind$1 = {})); - return NotificationKind$1; - })(); + var NotificationKind$1 = /*#__PURE__*/ (function (NotificationKind) { + NotificationKind["NEXT"] = "N"; + NotificationKind["ERROR"] = "E"; + NotificationKind["COMPLETE"] = "C"; + return NotificationKind; + })(NotificationKind$1 || {}); `, }), ); @@ -171,8 +212,7 @@ describe('adjust-typescript-enums Babel plugin', () => { * Supported http methods. * @deprecated use @angular/common/http instead */ - var RequestMethod = /*#__PURE__*/ (() => { - RequestMethod = RequestMethod || {}; + var RequestMethod = /*#__PURE__*/ (function (RequestMethod) { RequestMethod[(RequestMethod["Get"] = 0)] = "Get"; RequestMethod[(RequestMethod["Post"] = 1)] = "Post"; RequestMethod[(RequestMethod["Put"] = 2)] = "Put"; @@ -181,7 +221,7 @@ describe('adjust-typescript-enums Babel plugin', () => { RequestMethod[(RequestMethod["Head"] = 5)] = "Head"; RequestMethod[(RequestMethod["Patch"] = 6)] = "Patch"; return RequestMethod; - })(); + })(RequestMethod || {}); `, }), ); @@ -228,18 +268,17 @@ describe('adjust-typescript-enums Babel plugin', () => { })(ChangeDetectionStrategy || (ChangeDetectionStrategy = {})); `, expected: ` - var ChangeDetectionStrategy = /*#__PURE__*/ (() => { - ChangeDetectionStrategy = ChangeDetectionStrategy || {}; + var ChangeDetectionStrategy = /*#__PURE__*/ (function (ChangeDetectionStrategy) { ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush"; ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default"; return ChangeDetectionStrategy; - })(); + })(ChangeDetectionStrategy || {}); `, }), ); it( - 'should not wrap TypeScript enums if the declaration identifier has been renamed to avoid collisions', + 'should wrap TypeScript enums if the declaration identifier has been renamed to avoid collisions', testCase({ input: ` var ChangeDetectionStrategy$1; @@ -249,13 +288,11 @@ describe('adjust-typescript-enums Babel plugin', () => { })(ChangeDetectionStrategy$1 || (ChangeDetectionStrategy$1 = {})); `, expected: ` - var ChangeDetectionStrategy$1 = /*#__PURE__*/ (() => { - (function (ChangeDetectionStrategy) { - ChangeDetectionStrategy[(ChangeDetectionStrategy["OnPush"] = 0)] = "OnPush"; - ChangeDetectionStrategy[(ChangeDetectionStrategy["Default"] = 1)] = "Default"; - })(ChangeDetectionStrategy$1 || (ChangeDetectionStrategy$1 = {})); - return ChangeDetectionStrategy$1; - })(); + var ChangeDetectionStrategy$1 = /*#__PURE__*/ (function (ChangeDetectionStrategy) { + ChangeDetectionStrategy[ChangeDetectionStrategy["OnPush"] = 0] = "OnPush"; + ChangeDetectionStrategy[ChangeDetectionStrategy["Default"] = 1] = "Default"; + return ChangeDetectionStrategy; + })(ChangeDetectionStrategy$1 || {}); `, }), );