SAP/ui5-typescript

Possible issue: too many classes are abstract?

akudev opened this issue · 5 comments

akudev commented

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

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

BenReim commented

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

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

  • 👍1
Thodd commented

The EventProvider now allows for instantiation:
SAP/openui5@41826d2

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.

  • 👍1
akudev commented

Fixed by SAP/openui5@41826d2 for EventProvider. If you notice the same issue for other classes, please open an issue directly in the OpenUI5 repo. Thanks!