SAP / ui5-typescript

Tooling to enable TypeScript support in SAPUI5/OpenUI5 projects
https://sap.github.io/ui5-typescript
Apache License 2.0
202 stars 28 forks source link

Documentation / Feature request: How to use controller extensions in TS? #420

Closed vonBrax closed 2 months ago

vonBrax commented 1 year ago

I'm not sure if this is explained somewhere, but I couldn't find any examples on how to use controller extension in typescript.

The documentation briefly goes into the subject here, but it is mostly focused on explaining how to use the "overrides" property in the controller extension (probably related to https://github.com/SAP/ui5-typescript/issues/332), but it doesn't show how to actually use the extension in the controller file. The linked documentation in the github issue says something about definining the extensions in the manifest file, but that doesn't sound like an optimal workflow, and we would still need to make sure that TS knows about the extension.

The problem that we are facing is that if we import the extension into our controller and assign it to a class property, then the babel-plugin-transform-modules-ui5 will move the extension initiation inside the constructor. Since the logic that handle controller extensions happens inside the Controller constructor, and the extensions are defined only after the super call, they are never seen by the parent Controller class and therefore never instantiated.

During the time that the babel-plugin-transform-modules-ui5 wasn't being actively developed on (before moving to the ui5-community repo), I've created an internal plugin based on that one to handle this and a few other cases. Back then I've added a custom jsdoc tag to mark the extensions and keep them in the "extends" object, thus preventing them to end up in the constructor, after the super call.

But now we decided to adopt the plugin again, and I could only come up with two solutions - which I've included in this code sandbox - but I'm not really happy with any of them:

So my questions are:

1) Is there already a solution on how to use controller extensions with typescript? If there is, where can we find some documentation or tutorials showing it? Ideally without having to declare our extensions in the manifest file...

2) Would it make sense to create a feature request on the plugin's repository asking for a way to prevent the plugin from moving some specific class properties to the constructor and rather keep them in the extend object? Or any other solution that could help us working with controller extensions

akudev commented 1 year ago

Hi @vonBrax, this topic has come up recently and we are actively looking into it. Nevertheless thanks for reporting it in detail with your current workarounds. This gives additional insight, confirms the need, and finally makes the problem listed here. The difficulty is that those extensions are not "some specific class properties", but could have any name, hence we need something more explicit.

vonBrax commented 1 year ago

Hey @akudev, thanks for replying. The way I fixed this problem before was creating a generic "preserve" (or whatever name) tag and decorator in the babel plugin, to let it know that these class members should not be moved around and instead should go directly to the extends object. The advantages are that the logic wouldn't depend on the name of the class property itself and that you could use this tag to flag any other property that you want to be left untouched. The disadvantage is that we would need to remember to "mark" these properties and it may not be immediately clear why the controller extensions are not working. But maybe that could be a relatively easy fix?

codeworrior commented 1 year ago

Our thinking goes into the same direction. Either we introduce a marker method on Controller.prototype, something like

   myExtension = this.registerExtension(MyExtensionClass)

that makes the babel-plugin-transform-modules-ui5 move the field into the extend call.

Or we use a stage-3 decorator for that purpose, which looks a bit nicer in the code. Only drawback is that we need a new delivery for TypeScript content (the decorator), which we don't have yet.

That's something we still have to decide on, as @akudev mentioned.

akudev commented 1 year ago

@vonBrax When you extended that transformer plugin to keep things outside the constructor which were annotated with this specific JSDoc tag you invented, how did you deal with the fact that from TypeScript perspective the extension class was assigned, not an instance?

You wrote:

/**
 * @keepThisOutOfTheConstructor
 */
this.routing = Routing;

someMethod() {
   this.routing.navigate(...); // TS would complain here, as it thinks "routing" is the class, not an instance
}

EDIT: oh, you mentioned a decorator as well, then maybe this one has created what was needed for TS.

vonBrax commented 1 year ago

That's actually a good point, I forgot we would still need to type cast it. The decorators we were using were just empty decorators, since the plugin support for decorators is very limited. I did work on a complete rewrite of the plugin, with better integration with other babel plugins and decorators support, but I never published it, I gave up when I saw that it was being actively developed again by the ui5-community 🤷‍♂️

Anyways, we were just type casting it, we were mainly focused on having a working code and there were only a few places where we had TS controllers with extensions. I think an ideal solution could be what @codeworrior mentioned, but I'm assuming it would take some time before it lands. For a short term fix, I'm imagining something like:

/**
 * @controllerextension
 */
this.routing = new Routing();

someMethod() {
  this.routing.navigate(...); // OK, TS doesn't complain
}

Then in the plugin, for properties marked with the @controllerextension tag, convert the babel NewExpression to an Identifier, so we would end up with:

....extend('com.example.controller', {
  ...,
  routing: Routing,
  ...
});

But I imagine some people woudln't agree with that, since we are changing the meaning of the code with the babel plugin.

I still think it would be a good idea to have a generic tag to preserve some properties, though. We were using it both for the controller extensions and inside the controller extension for the override property, and maybe there could be even more use cases where it would be helpful.

dfenerski commented 1 year ago

It has always worked for me, am I missing something?

.babelrc.json:

    "presets": [
        [
            "transform-ui5",
            {
                "onlyMoveClassPropsUsingThis": true
            }
        ],
        ["@babel/preset-typescript", {}]
    ]

Stuff.extension.ts:

/**
 * @namespace my.ns.foo.bar
 */
class StuffExtension
    extends ControllerExtension{
// this works
public static metadata = { ... };
// this works too
public static override = { ... };
// add any methods as normal
public foo(){
    throw 'bar';
}

// anything else

}
// Lie to the type system because ui5 will instantiate the extension at runtime
export default StuffExtension as unknown as UI5ControllerExtension<
    typeof StuffExtension
>;

UI5ControllerExtension.ts:

export type UI5ControllerExtension<T extends new (...args: any) => any> =
    InstanceType<T> & {
        override(...args: any[]): InstanceType<T> & { override(): never };
    };

Demo.controller.ts:

    public readonly StuffExtension = StuffExtension.override({
        foo: function () { doWork() },
    });
   // just `public readonly StuffExtension = StuffExtension` works ofc too

The only problem I have with it does not relate to TS - ControllerExtension does not allow subclassing. This hinders reusal. AFAIK it has something to do with the fact it inherits from BaseObject which is the earlies object in the main hierarchy but can't really say as I haven't done the time to research this more thoroughly.

akudev commented 1 year ago

@dfenerski Well, the "Lie to the type system" part is something usually not found in the Controller Extensions provided by Fiori Elements. That's what I meant in my comment elsewhere. The rest does work when you use onlyMoveClassPropsUsingThis (which, as I stated there as well, is probably not meant to be how to best use the transformer, as this is only a compatibility switch to restore the legacy behavior). So yes, with this type hack and the compatibility switch I guess it works.

The extension issue you also reported sounds strange. Even sap.ui.base.Object, from which it inherits, has the extend method and I don't see any attempt in its code to prevent this for subclasses.

akudev commented 11 months ago

While that other change provided support for the main use-case, the solution is not completely covering extended use-cases and people are still looking into comprehensive support and documentation, so I'll re-open this.

akudev commented 2 months ago

This is now completed by a series of changes across repos, like:

See https://sap.github.io/ui5-typescript/releasenotes.html (version 1.127) and https://github.com/ui5-community/babel-plugin-transform-modules-ui5?tab=readme-ov-file#properties-related-to-controller-extensions for usage.