Skip to content

Conversation

devinea
Copy link
Member

@devinea devinea commented Sep 22, 2025

No description provided.

@devinea devinea requested a review from a team as a code owner September 22, 2025 16:10
Copy link

changeset-bot bot commented Sep 22, 2025

🦋 Changeset detected

Latest commit: c704543

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@sap-ux/fe-fpm-writer Patch
@sap-ux/fiori-elements-writer Patch
@sap-ux/fe-fpm-cli Patch
@sap-ux/fiori-app-sub-generator Patch
@sap-ux/repo-app-import-sub-generator Patch
@sap-ux/generator-simple-fe Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

devinea and others added 3 commits September 22, 2025 18:17
… into update_ControllerExtension.d.ts

* origin/update_ControllerExtension.d.ts:
  Create serious-beans-dream.md
Copy link

Copy link
Contributor

@heimwege heimwege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments. Coming with proper types out-of-the box would really be a great benefit for TS based controller extensions. So thanks for this PR 👍🏻


export default class ControllerExtension<ExtensionAPI> {
override?: Overrides;
base: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I use

import type PageController from "sap/fe/core/PageController";
//[...]
protected base!: PageController;

as type in my controller extension. I don't think you need to list all PageController functions if you just re-use that type. And it also comes with less maintenance in case the PageController type changes later on.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing that came into my mind. Actually base can be the actual controller that is extended (ListReportController or ObjectPageController), that itself returns the respective ExtensionAPI (LR or OP ExtensionAPI), and some additional APIs.

https://ui5.sap.com/#/api/sap.fe.templates

The ObjectPageController is missing from our public API, but I'd say we should make it public as well (as it can also be extended already) - any objections @nlunets ?

};
}

export type Overrides<GenericController extends Controller = Controller> = {
Copy link
Contributor

@heimwege heimwege Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the overrides I use

import ViewState from "sap/fe/core/controllerextensions/ViewState";
import EditFlow from "sap/fe/core/controllerextensions/EditFlow";
import Routing from "sap/fe/core/controllerextensions/Routing";
import Controller from "sap/ui/core/mvc/Controller";

type ControllerExtensionOverrides = Partial<Pick<typeof Controller["prototype"], "onAfterRendering" | "onBeforeRendering" | "onExit" | "onInit">>;

/**
 * Types for Fiori Elements Controller Extension Overrides (sap.ui.core.mvc.ControllerExtension.override).
 */
type FioriElementsControllerExtensionOverrides = ControllerExtensionOverrides & {
    /**
     * experimental
     */
    onPageReady?: () => void;
    editFlow?: Partial<Pick<typeof EditFlow["prototype"], "onAfterCreate" | "onAfterDelete" | "onAfterDiscard" | "onAfterEdit" | "onAfterSave" | "onBeforeCreate" | "onBeforeDelete" | "onBeforeDiscard" | "onBeforeEdit" | "onBeforeSave">>;
    routing?: Partial<Pick<typeof Routing["prototype"], "onAfterBinding" | "onBeforeBinding" | "onBeforeNavigation">>;
    viewState?: Partial<Pick<typeof ViewState["prototype"], "adaptBindingRefreshControls" | "adaptBindingRefreshHandler" | "adaptControlStateHandler" |"adaptStateControls" | "applyAdditionalStates" | "applyInitialStateOnly" | "applyNavigationParameters" | "onAfterStateApplied" | "onBeforeStateApplied" | "retrieveAdditionalStates">>;
};

in my controller extension.
My definition might be incomplete though. But I think just having every property optional actually is not the correct type for overrides because you cannot override everything from that type. So code completion gives you wrong suggestions. Downside is of course that newly introduced properties are missing in the type definition.
To overcome this nowadays I probably would introduce a utility type to filter on e.g. event handers that start with on and pick those.

/**
 * A utility type that extracts the keys of an object `T` that correspond to methods
 * starting with the prefix "on".
 */
type OnMethodKeys<T> = {
  [K in keyof T]: K extends `on${string}` // 1. If the key starts with "on"...
    ? T[K] extends (...args: any[]) => any // 2. ...and the property is a function...
      ? K // ...keep the key.
      : never
    : never; // 3. Discard all other keys.
}[keyof T]; // 4. Collect the kept keys into a union type.

//[...]

editFlow?: Partial<Pick<typeof EditFlow["prototype"], OnMethodKeys<typeof EditFlow["prototype"]>>>;

@MarcelWaechter But actually I would prefer if Fiori elements would provide those types (UI5 version dependent). This would be a great feature, though I understand the JSDoc-based type generation is a hurdle 🙈

(Please note that onPageReady is experimental and should maybe not be part in an official type definition.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah onPageReady is not a public one. Recommendations also not yet public.

EditFlow, IntentBasedNavigation, MessageHandler, Paginator, Routing, Share and ViewState is fine.

Yeah let's see how we can improve as well, also adding @nlunets .
Now as we want to provide the Fiori Dev Portal examples with TypeScript, we have the same pain 😆

Copy link
Contributor

@heimwege heimwege Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I further refined this. What could be handy is something like:

import ViewState from "sap/fe/core/controllerextensions/ViewState";
import EditFlow from "sap/fe/core/controllerextensions/EditFlow";
import Routing from "sap/fe/core/controllerextensions/Routing";
import Controller from "sap/ui/core/mvc/Controller";

/**
 * A utility type that extracts the keys of an object `T` that correspond to methods
 * starting with the specified prefix `P`.
 * 
 * @template T - The object type to extract method keys from
 * @template P - The string prefix to match (e.g., "on", "adapt", "apply", "retrieve")
 */
type MethodKeys<T, P extends string> = {
  [K in keyof T]: K extends `${P}${string}` // 1. If the key starts with the prefix...
    ? T[K] extends (...args: any[]) => any // 2. ...and the property is a function...
      ? K // ...keep the key.
      : never
    : never; // 3. Discard all other keys.
}[keyof T]; // 4. Collect the kept keys into a union type.

/**
 * A utility type that combines method keys with multiple prefixes.
 * 
 * @template T - The object type to extract method keys from
 * @template P - A union of string prefixes to match
 */
type MultiPrefixMethodKeys<T, P extends string> = P extends string 
  ? MethodKeys<T, P> 
  : never;

/**
 * Types for UI5 Controller Extension Overrides (sap.ui.core.mvc.ControllerExtension.override).
 */
type ControllerExtensionOverrides = Partial<Pick<typeof Controller["prototype"], MethodKeys<typeof Controller["prototype"], "on">>>;

/**
 * Types for Fiori Elements Controller Extension Overrides (sap.ui.core.mvc.ControllerExtension.override).
 */
type FioriElementsControllerExtensionOverrides = ControllerExtensionOverrides & {
    /**
     * experimental
     */
    onPageReady?: () => void;
    editFlow?: Partial<Pick<typeof EditFlow["prototype"], MethodKeys<typeof EditFlow["prototype"], "on">>>;
    routing?: Partial<Pick<typeof Routing["prototype"], MethodKeys<typeof Routing["prototype"], "on">>>;
    viewState?: Partial<Pick<typeof ViewState["prototype"], MultiPrefixMethodKeys<typeof ViewState["prototype"], "on" | "adapt" | "apply" | "retrieve">>>;
};

If Fiori elements keeps following those function naming pattern this should be fine.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, the fact a method starts with on doesn't say it's public and can be overridden. That's defined in the decorators, like

@publicExtension()
@extensible("AfterAsync")
async onBeforeSave(_mParameters?: { context?: Context }): Promise {
// to be overridden
return Promise.resolve();
}

I guess you can't read our decorators... but it's there in the metadata, just checked quickly

image

but yes not great, it shall be ideally accessible through the TS types...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants