search

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

Possible issue: too many classes are abstract? #435

Closed akudev closed 7 months ago

akudev commented 7 months ago

Feedback from @BenReim: sometimes it is needed to create an instance of such a base class, which is now marked as abstract. Even in UI5 framework code, EventProvider instances are created, which in TypeScript would no longer work now: https://github.com/search?q=repo%3ASAP%2Fopenui5%20new%20EventProvider()&type=code

@akudev reply: abstract might be wrong in some cases, but that's then a bug in the UI5 sources (JSDoc), which are used to generate the types. Not in the type generator. But of course it can simply be ignored in JS, but not in TypeScript. Which classes do you have in mind?

@BenReim reply: Only noticed for EventProvider. Probably instantiating should also be possible for Object and ManagedObject.

Let's use this issue to discuss and agree on the way forward.

codeworrior commented 7 months ago

Bringing this to the attention of @loginger and @Thodd.

BenReim commented 7 months ago

Hi team, I'm affected by this in a current use case and was wondering if you could share any update or ideas moving forward? Thanks for your work on ui5 and the typings!

Thodd commented 7 months ago

Hi @BenReim,

sorry for the late reply, I was OOO for the last 2 weeks and didn't see @codeworrior tag me.

I just prepared a small change to remove the abstract from the EventProvider, since it is also used internally quite often to facilitate eventing without the need to expose the full API and/or inherit from it.

I'll keep an eye on this topic and we will discuss the other proposals by akudev. The ManagedObject is also instantiated quite often in QUnit tests, but here the metadata always has explicitly defined it as "abstract: true" (unlike the EventProvider, which doesn't).

BR,

Thorsten

Thodd commented 7 months ago

The EventProvider now allows for instantiation: https://github.com/SAP/openui5/commit/41826d2d7b68818375493843665f11f0d79f4f6e

Though we don't want to allow instantiation of ManagedObject and/or base.Object. Both should have no need to be instantiated.

We see a lot of tests using it though... mainly to have a stub-like thing they can add to an aggregation. The tests should actually be migrated to using real controls instead of an empty ManagedObject.

akudev commented 7 months ago

Fixed by https://github.com/SAP/openui5/commit/41826d2d7b68818375493843665f11f0d79f4f6e for EventProvider. If you notice the same issue for other classes, please open an issue directly in the OpenUI5 repo. Thanks!