From c34479410d007834b0e04f4d21624cd16870e2e1 Mon Sep 17 00:00:00 2001 From: akudev Date: Thu, 12 Sep 2024 18:37:19 +0200 Subject: [PATCH 1/2] fix: support wrapped controller extension assignments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code instrumentation (for coverage measurement) may wrap controller extension assignments like: this.routing = (cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing.override({ … }))); --- .../__test__/__snapshots__/test.js.snap | 18 +++++ .../ts-class-controller-extension-wrapped.ts | 17 +++++ .../plugin/src/classes/helpers/classes.js | 73 ++++++++++++------- 3 files changed, 82 insertions(+), 26 deletions(-) create mode 100644 packages/plugin/__test__/fixtures/typescript/ts-class-controller-extension-wrapped.ts diff --git a/packages/plugin/__test__/__snapshots__/test.js.snap b/packages/plugin/__test__/__snapshots__/test.js.snap index 4aadd7b..bb6b857 100644 --- a/packages/plugin/__test__/__snapshots__/test.js.snap +++ b/packages/plugin/__test__/__snapshots__/test.js.snap @@ -1875,6 +1875,24 @@ exports[`typescript ts-class-controller-extension-usage-new.ts 1`] = ` });" `; +exports[`typescript ts-class-controller-extension-wrapped.ts 1`] = ` +"sap.ui.define(["sap/ui/core/mvc/Controller", "sap/ui/core/mvc/ControllerExtension", "sap/fe/core/controllerextensions/Routing"], function (Controller, ControllerExtension, Routing) { + "use strict"; + + const cov_1uvvg22e7l = () => { + return { + "s": {} + }; + }; + const MyExtendedController = Controller.extend("test.controller.MyExtendedController", { + routing3: Routing.override({}), + routing2: Routing.override({}), + routing: Routing + }); + return MyExtendedController; +});" +`; + exports[`typescript ts-class-param-props.ts 1`] = ` "sap.ui.define(["sap/Class"], function (SAPClass) { "use strict"; 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 new file mode 100644 index 0000000..17a7604 --- /dev/null +++ b/packages/plugin/__test__/fixtures/typescript/ts-class-controller-extension-wrapped.ts @@ -0,0 +1,17 @@ +import Controller from "sap/ui/core/mvc/Controller"; +import ControllerExtension from "sap/ui/core/mvc/ControllerExtension"; +import Routing from "sap/fe/core/controllerextensions/Routing"; + +const cov_1uvvg22e7l = () => { return { "s": {} }; }; // dummy coverage function + +/** + * @namespace test.controller + */ +export default class MyExtendedController extends Controller { + + // code could already be instrumented, e.g. for code coverage by istanbul, and look like this: + //this.routing = (cov_1uvvg22e7l().s[5]++, ControllerExtension.use(Routing.override({ … }))); + 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({}))); +} \ 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 ed37e6f..a537e44 100644 --- a/packages/plugin/src/classes/helpers/classes.js +++ b/packages/plugin/src/classes/helpers/classes.js @@ -222,37 +222,58 @@ 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. - if (t.isCallExpression(member.value)) { - const callee = member.value.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 ( + t.isCallExpression(member.value) || + t.isSequenceExpression(member.value) + ) { + 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 ( - importDeclaration?.source?.value === - "sap/ui/core/mvc/ControllerExtension" + 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 ( - !member.value.arguments || - member.value.arguments.length !== 1 + importDeclaration?.source?.value === + "sap/ui/core/mvc/ControllerExtension" ) { - // exactly one argument must be there - throw memberPath.buildCodeFrameError( - `ControllerExtension.use() must be called with exactly one argument but has ${ - member.value.arguments ? member.value.arguments.length : 0 - }` - ); + 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 = member.value.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 } } } From aaa6e217cc1a2829a4db9ccf45c5f67c8182af6b Mon Sep 17 00:00:00 2001 From: akudev Date: Fri, 13 Sep 2024 11:58:54 +0200 Subject: [PATCH 2/2] 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.