From aaa6e217cc1a2829a4db9ccf45c5f67c8182af6b Mon Sep 17 00:00:00 2001 From: akudev Date: Fri, 13 Sep 2024 11:58:54 +0200 Subject: [PATCH] fix: fix review comments - maintain full SequenceExpression --- .../__test__/__snapshots__/test.js.snap | 7 +- .../ts-class-controller-extension-wrapped.ts | 1 + .../plugin/src/classes/helpers/classes.js | 109 +++++++++--------- 3 files changed, 62 insertions(+), 55 deletions(-) diff --git a/packages/plugin/__test__/__snapshots__/test.js.snap b/packages/plugin/__test__/__snapshots__/test.js.snap index bb6b857..b44f49a 100644 --- a/packages/plugin/__test__/__snapshots__/test.js.snap +++ b/packages/plugin/__test__/__snapshots__/test.js.snap @@ -1885,9 +1885,10 @@ exports[`typescript ts-class-controller-extension-wrapped.ts 1`] = ` }; }; const MyExtendedController = Controller.extend("test.controller.MyExtendedController", { - routing3: Routing.override({}), - routing2: Routing.override({}), - routing: Routing + routing4: (cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing.override({})), ControllerExtension.use(Routing)), + routing3: (cov_1uvvg22e7l().s[5]++, cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing.override({}))), + routing2: (cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing.override({}))), + routing: (cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing)) }); return MyExtendedController; });" diff --git a/packages/plugin/__test__/fixtures/typescript/ts-class-controller-extension-wrapped.ts b/packages/plugin/__test__/fixtures/typescript/ts-class-controller-extension-wrapped.ts index 17a7604..852e6db 100644 --- a/packages/plugin/__test__/fixtures/typescript/ts-class-controller-extension-wrapped.ts +++ b/packages/plugin/__test__/fixtures/typescript/ts-class-controller-extension-wrapped.ts @@ -14,4 +14,5 @@ export default class MyExtendedController extends Controller { routing = (cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing)); routing2 = (cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing.override({}))); routing3 = (cov_1uvvg22e7l().s[5]++, cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing.override({}))); + routing4 = (cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing.override({})), ControllerExtension.use(Routing)); } \ No newline at end of file diff --git a/packages/plugin/src/classes/helpers/classes.js b/packages/plugin/src/classes/helpers/classes.js index a537e44..ffbf10d 100644 --- a/packages/plugin/src/classes/helpers/classes.js +++ b/packages/plugin/src/classes/helpers/classes.js @@ -222,60 +222,24 @@ export function convertClassToUI5Extend( // @transformControllerExtension marker, because it is not a type assignment. In the resulting code, the // "ControllerExtension.use(...)" part should be removed and the content of the brackets should be assigned // directly to the member property. + const rightSide = member.value; + if (isCallToControllerExtensionUse(rightSide, memberPath)) { + member.value = rightSide.arguments[0]; + extendProps.unshift(buildObjectProperty(member)); // add it to the properties of the extend() config object + continue; // prevent the member from also being added to the constructor + } + + // code instrumentation sometimes wraps ControllerExtension.use() calls like: + // this.routing = (cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing.override({ … }))); if ( - t.isCallExpression(member.value) || - t.isSequenceExpression(member.value) + t.isSequenceExpression(rightSide) && + rightSide.expressions.some((expression) => + isCallToControllerExtensionUse(expression, memberPath) + ) ) { - let callExpression = member.value; - - // code instrumentation sometimes wraps it like: - // this.routing = (cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing.override({ … }))); - if (t.isSequenceExpression(member.value)) { - // iterate through the expressions in the sequence - for (const expr of member.value.expressions) { - if (t.isCallExpression(expr)) { - callExpression = expr; - break; - } - } - } - - if (t.isCallExpression(callExpression)) { - const callee = callExpression.callee; - if ( - t.isMemberExpression(callee) && - t.isIdentifier(callee.object) && - t.isIdentifier(callee.property) && - callee.property.name === "use" // we are looking for "ControllerExtension.use(...)" - ) { - const importDeclaration = getImportDeclaration( - memberPath?.hub?.file?.opts?.filename, - callee?.object?.name // usually, but not necessarily always: "ControllerExtension"... - ); - // ...hence we rather look at the imported module name to be sure - if ( - importDeclaration?.source?.value === - "sap/ui/core/mvc/ControllerExtension" - ) { - if ( - !callExpression.arguments || - callExpression.arguments.length !== 1 - ) { - // exactly one argument must be there - throw memberPath.buildCodeFrameError( - `ControllerExtension.use() must be called with exactly one argument but has ${ - callExpression.arguments - ? callExpression.arguments.length - : 0 - }` - ); - } - member.value = callExpression.arguments[0]; - extendProps.unshift(buildObjectProperty(member)); // add it to the properties of the extend() config object - continue; // prevent the member from also being added to the constructor - } - } - } + member.value = rightSide; + extendProps.unshift(buildObjectProperty(member)); // add it to the properties of the extend() config object + continue; // prevent the member from also being added to the constructor } // Special handling for TypeScript limitation where metadata, renderer and overrides must be properties. @@ -314,6 +278,47 @@ export function convertClassToUI5Extend( } } + /** + * Checks whether the given thing is a CallExpression that calls ControllerExtension.use(...) + * + * @param {*} expression the thing to check - does not need to be a CallExpression + * @param {string} memberPath + * @returns true if the given expression is a CallExpression that calls ControllerExtension.use(...) + */ + function isCallToControllerExtensionUse(expression, memberPath) { + if (!t.isCallExpression(expression)) { + return false; + } + const callee = expression.callee; + if ( + t.isMemberExpression(callee) && + t.isIdentifier(callee.object) && + t.isIdentifier(callee.property) && + callee.property.name === "use" // we are looking for "ControllerExtension.use(...)" + ) { + const importDeclaration = getImportDeclaration( + memberPath?.hub?.file?.opts?.filename, + callee?.object?.name // usually, but not necessarily always: "ControllerExtension"... + ); + // ...hence we rather look at the imported module name to be sure + if ( + importDeclaration?.source?.value === + "sap/ui/core/mvc/ControllerExtension" + ) { + if (!expression.arguments || expression.arguments.length !== 1) { + // exactly one argument must be there + throw memberPath.buildCodeFrameError( + `ControllerExtension.use() must be called with exactly one argument but has ${ + expression.arguments ? expression.arguments.length : 0 + }` + ); + } + return true; + } + } + return false; // return false if not a match + } + // Arrow function properties need to get moved to the constructor so that // they're bound properly to the class instance, to align with the spec. // For controllers, use onInit rather than constructor, since controller constructors don't work.