Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
vonBrax opened this issue Oct 23, 2023 · 8 comments · Fixed by ui5-community/babel-plugin-transform-modules-ui5#120
Labels
bug Something isn't working enhancement New feature or request

Comments

@vonBrax
Copy link

vonBrax commented Oct 23, 2023

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 #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:

  • Mark the extension as static, while setting the plugin option addControllerStaticPropsToExtend to true in the babel config (or ui5.yaml). The extension works, but TS now thinks that the extension is a static member instead of an instance member, so we need some not very elegant type casting;
  • Leave the extension as an instance member and handle the extension initialization ourselves - using the private extendByMember method from the Controller class linked above. This also works, but we are using a private method (and TS doesn't know about it) and we also need to make sure to call that method in the constructor of every controller class that uses extensions.

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
Copy link
Contributor

akudev commented Oct 24, 2023

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
Copy link
Author

vonBrax commented Oct 24, 2023

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
Copy link
Member

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
Copy link
Contributor

akudev commented Oct 24, 2023

@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
Copy link
Author

vonBrax commented Oct 24, 2023

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.

@akudev akudev added bug Something isn't working enhancement New feature or request labels Oct 26, 2023
@dfenerski
Copy link

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
Copy link
Contributor

akudev commented Nov 14, 2023

@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 added a commit to akudev/babel-plugin-transform-modules-ui5 that referenced this issue Nov 28, 2023
When using a controller extension in your own controller as a kind of
mix-in, UI5 currently expects in the "extend" config object the
extension class to be assigned to a property.

In TypeScript, however, the TS compiler must see the property being
typed with an *instance* of the extension class, so subsequent calls to
the extension can be written like they should
(this.routing.doSomething()).

This change introduces the "@transformControllerExtension" comment and
decorator which triggers a special kind of transformation for annotated
class properties. The controller extension type assigned to the
subsequent class property is then assigned as class in the extend object
in JavaScript. And this assignment is not moved into the constructor
like it normally would be.

Fixes SAP/ui5-typescript#420
akudev added a commit to ui5-community/babel-plugin-transform-modules-ui5 that referenced this issue Nov 30, 2023
When using a controller extension in your own controller as a kind of
mix-in, UI5 currently expects in the "extend" config object the
extension class to be assigned to a property.

In TypeScript, however, the TS compiler must see the property being
typed with an *instance* of the extension class, so subsequent calls to
the extension can be written like they should
(this.routing.doSomething()).

This change introduces the "@transformControllerExtension" comment and
decorator which triggers a special kind of transformation for annotated
class properties. The controller extension type assigned to the
subsequent class property is then assigned as class in the extend object
in JavaScript. And this assignment is not moved into the constructor
like it normally would be.

Fixes SAP/ui5-typescript#420
@akudev
Copy link
Contributor

akudev commented Dec 28, 2023

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 akudev reopened this Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants