From 4d7cd5e3ed303c53b2cc63720b9a577e2f46f170 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Tue, 21 May 2024 09:08:16 +0000 Subject: [PATCH] fix(@angular/build): correctly wrap class expressions with static properties or blocks emitted by esbuild Prior to this commit, class expressions emitted by esbuild that have static blocks or properties were not being properly wrapped. This caused the class to become non-treeshakable. Closes #27662 (cherry picked from commit ac9fc7265d6de7927b34c7b4985c546da16d1288) --- .../plugins/adjust-static-class-members.ts | 174 +++++++++--------- .../adjust-static-class-members_spec.ts | 27 +++ 2 files changed, 117 insertions(+), 84 deletions(-) diff --git a/packages/angular/build/src/tools/babel/plugins/adjust-static-class-members.ts b/packages/angular/build/src/tools/babel/plugins/adjust-static-class-members.ts index d7791a1f8454..adf169e65509 100644 --- a/packages/angular/build/src/tools/babel/plugins/adjust-static-class-members.ts +++ b/packages/angular/build/src/tools/babel/plugins/adjust-static-class-members.ts @@ -245,87 +245,12 @@ export default function (): PluginObj { visitedClasses.add(classNode); - if (hasPotentialSideEffects) { - return; - } - // If no statements to wrap, check for static class properties. - // Static class properties may be downleveled at later stages in the build pipeline - // which results in additional function calls outside the class body. These calls - // then cause the class to be referenced and not eligible for removal. Since it is - // not known at this stage whether the class needs to be downleveled, the transform - // wraps classes preemptively to allow for potential removal within the optimization - // stages. - if (wrapStatementPaths.length === 0) { - let shouldWrap = false; - for (const element of path.get('body').get('body')) { - if (element.isClassProperty()) { - // Only need to analyze static properties - if (!element.node.static) { - continue; - } - - // Check for potential side effects. - // These checks are conservative and could potentially be expanded in the future. - const elementKey = element.get('key'); - const elementValue = element.get('value'); - if ( - elementKey.isIdentifier() && - (!elementValue.isExpression() || - canWrapProperty(elementKey.node.name, elementValue)) - ) { - shouldWrap = true; - } else { - // Not safe to wrap - shouldWrap = false; - break; - } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } else if ((element as any).isStaticBlock()) { - // Only need to analyze static blocks - const body = element.get('body'); - - if (Array.isArray(body) && body.length > 1) { - // Not safe to wrap - shouldWrap = false; - break; - } - - const expression = body.find((n: NodePath) => - n.isExpressionStatement(), - ) as NodePath | undefined; - - const assignmentExpression = expression?.get('expression'); - if (assignmentExpression?.isAssignmentExpression()) { - const left = assignmentExpression.get('left'); - if (!left.isMemberExpression()) { - continue; - } - - if (!left.get('object').isThisExpression()) { - // Not safe to wrap - shouldWrap = false; - break; - } - - const element = left.get('property'); - const right = assignmentExpression.get('right'); - if ( - element.isIdentifier() && - (!right.isExpression() || canWrapProperty(element.node.name, right)) - ) { - shouldWrap = true; - } else { - // Not safe to wrap - shouldWrap = false; - break; - } - } - } - } - if (!shouldWrap) { - return; - } + if ( + hasPotentialSideEffects || + (wrapStatementPaths.length === 0 && !analyzeClassStaticProperties(path).shouldWrap) + ) { + return; } const wrapStatementNodes: types.Statement[] = []; @@ -359,9 +284,7 @@ export default function (): PluginObj { const { node: classNode, parentPath } = path; const { wrapDecorators } = state.opts as { wrapDecorators: boolean }; - // Class expressions are used by TypeScript to represent downlevel class/constructor decorators. - // If not wrapping decorators, they do not need to be processed. - if (!wrapDecorators || visitedClasses.has(classNode)) { + if (visitedClasses.has(classNode)) { return; } @@ -382,7 +305,11 @@ export default function (): PluginObj { visitedClasses.add(classNode); - if (hasPotentialSideEffects || wrapStatementPaths.length === 0) { + // If no statements to wrap, check for static class properties. + if ( + hasPotentialSideEffects || + (wrapStatementPaths.length === 0 && !analyzeClassStaticProperties(path).shouldWrap) + ) { return; } @@ -416,3 +343,82 @@ export default function (): PluginObj { }, }; } + +/** + * Static class properties may be downleveled at later stages in the build pipeline + * which results in additional function calls outside the class body. These calls + * then cause the class to be referenced and not eligible for removal. Since it is + * not known at this stage whether the class needs to be downleveled, the transform + * wraps classes preemptively to allow for potential removal within the optimization stages. + */ +function analyzeClassStaticProperties( + path: NodePath, +): { shouldWrap: boolean } { + let shouldWrap = false; + for (const element of path.get('body').get('body')) { + if (element.isClassProperty()) { + // Only need to analyze static properties + if (!element.node.static) { + continue; + } + + // Check for potential side effects. + // These checks are conservative and could potentially be expanded in the future. + const elementKey = element.get('key'); + const elementValue = element.get('value'); + if ( + elementKey.isIdentifier() && + (!elementValue.isExpression() || canWrapProperty(elementKey.node.name, elementValue)) + ) { + shouldWrap = true; + } else { + // Not safe to wrap + shouldWrap = false; + break; + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } else if ((element as any).isStaticBlock()) { + // Only need to analyze static blocks + const body = element.get('body'); + + if (Array.isArray(body) && body.length > 1) { + // Not safe to wrap + shouldWrap = false; + break; + } + + const expression = body.find((n: NodePath) => n.isExpressionStatement()) as + | NodePath + | undefined; + + const assignmentExpression = expression?.get('expression'); + if (assignmentExpression?.isAssignmentExpression()) { + const left = assignmentExpression.get('left'); + if (!left.isMemberExpression()) { + continue; + } + + if (!left.get('object').isThisExpression()) { + // Not safe to wrap + shouldWrap = false; + break; + } + + const element = left.get('property'); + const right = assignmentExpression.get('right'); + if ( + element.isIdentifier() && + (!right.isExpression() || canWrapProperty(element.node.name, right)) + ) { + shouldWrap = true; + } else { + // Not safe to wrap + shouldWrap = false; + break; + } + } + } + } + + return { shouldWrap }; +} diff --git a/packages/angular/build/src/tools/babel/plugins/adjust-static-class-members_spec.ts b/packages/angular/build/src/tools/babel/plugins/adjust-static-class-members_spec.ts index 83f4dfeee697..40cca69f8af3 100644 --- a/packages/angular/build/src/tools/babel/plugins/adjust-static-class-members_spec.ts +++ b/packages/angular/build/src/tools/babel/plugins/adjust-static-class-members_spec.ts @@ -595,6 +595,33 @@ describe('adjust-static-class-members Babel plugin', () => { }), ); + it( + 'wraps class with Angular ɵfac static field (esbuild)', + testCase({ + input: ` + var Comp2Component = class _Comp2Component { + static { + this.ɵfac = function Comp2Component_Factory(t) { + return new (t || _Comp2Component)(); + }; + } + }; + `, + expected: ` + var Comp2Component = /*#__PURE__*/ (() => { + let Comp2Component = class _Comp2Component { + static { + this.ɵfac = function Comp2Component_Factory(t) { + return new (t || _Comp2Component)(); + }; + } + }; + return Comp2Component; + })(); + `, + }), + ); + it( 'wraps class with class decorators when wrapDecorators is true (esbuild output)', testCase({