Skip to content

Commit

Permalink
fix(@angular-devkit/build-angular): correctly wrap CommonJS exported …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@clydin
clydin committed Jul 13, 2023
1 parent 953104e commit 5908ede
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 82 deletions.
74 changes: 26 additions & 48 deletions packages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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.
Expand All @@ -100,56 +103,31 @@ export default function (): PluginObj {
}

hasElements = true;
enumAssignments?.push(enumStatement.node);
}

// If there are no enum elements then there is nothing to wrap
if (!hasElements) {
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);
},
},
};
Expand Down
105 changes: 71 additions & 34 deletions ...ages/angular_devkit/build_angular/src/tools/babel/plugins/adjust-typescript-enums_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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 || {});
`,
}),
);
Expand All @@ -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 = {}));
`,
}),
);
Expand All @@ -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 || {});
`,
}),
);
Expand All @@ -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 || {});
`,
}),
);
Expand All @@ -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 || {});
`,
}),
);
Expand Down Expand Up @@ -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";
Expand All @@ -181,7 +221,7 @@ describe('adjust-typescript-enums Babel plugin', () => {
RequestMethod[(RequestMethod["Head"] = 5)] = "Head";
RequestMethod[(RequestMethod["Patch"] = 6)] = "Patch";
return RequestMethod;
})();
})(RequestMethod || {});
`,
}),
);
Expand Down Expand Up @@ -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;
Expand All @@ -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 || {});
`,
}),
);
Expand Down

0 comments on commit 5908ede

Please sign in to comment.