Feature request: adjust getSource method return type for sap.ui.base.Event

iljapostnovs commented 1 year ago

As of UI5 TS types v1.115.1 separate event types were introduced. As a result, event parameters are typed and it's great! Big shout out for this feature :) That also opened a room for me to introduce automatic inserting of that event type with VSCode SAPUI5 Extension if the event handler is created from XML.

However, if there is a separate type created for every event, It would be great if getSource method would return the necessary control as well. I believe that now it is much easier to add this behavior to the Event class with generics.

Expected behavior oList should be ListBase instead of EventProvider.

Thanks!

akudev commented 1 year ago

Sounds entirely reasonable. Let's have a look what this means for the implementation.

iljapostnovs commented 1 year ago

@akudev, I had a bit of sleepover with this thought and another idea came into my mind. As an option, I would like to propose to add event types for all children of the classes as well. Example:

import { List$SelectionChangeEvent } from "sap/m/List";
onListOrdersSelectionChange(oEvent: List$SelectionChangeEvent) {
    const oList = oEvent.getSource(); // (oList: sap.m.List)
}

That would be beneficial for two reasons:

oList would be instance of sap.m.List, not sap.m.ListBase, and it would be more precise type.
From the developers perspective, it is easier to "generate" the name of the event type right away without checking parent classes. Currently flow of determining the correct event type looks as follows:
1. I have e.g. sap.m.List -> selectionChange event
2. Check if List$SelectionChangeEvent exists
3. It doesn't exist, so I go to the UI5 API to check from which class was this event inherited from
4. When I find that the owner of the event is sap.m.ListBase, only then I can come up with ListBase$SelectionChangeEvent.
  If types for ListBase children are added, It will be possible to know the event type from point No. 1.

Please share your thoughts if it would be possible.

akudev commented 1 year ago

@iljapostnovs It would certainly be helpful for obvious cases of "event is defined in a base class belonging to the same family of controls" like InputBase$Change. But less so for Events defined for a broader range of controls/Classes, where one would not expect to find the event on a specific class. Every ManagedObject inherits five events, every control at least six events from a base class, like "modelContextUpdate" "validationFailed" etc. Right now I don't see that we want to define all these and more events for every single class inheriting from ManagedObject. We might find a solution for returning the proper type, but as long as it's about identifying the event, editors are smart enough to suggest "InputBase$ChangeEvent" when you type "Input$Change", so the "need to go to the API documentation" argument doesn't really look valid to me.

iljapostnovs commented 1 year ago

@iljapostnovs It would certainly be helpful for obvious cases of "event is defined in a base class belonging to the same family of controls" like InputBase$Change. But less so for Events defined for a broader range of controls/Classes, where one would not expect to find the event on a specific class. Every ManagedObject inherits five events, every control at least six events from a base class, like "modelContextUpdate" "validationFailed" etc. Right now I don't see that we want to define all these and more events for every single class inheriting from ManagedObject. We might find a solution for returning the proper type, but as long as it's about identifying the event, editors are smart enough to suggest "InputBase$ChangeEvent" when you type "Input$Change", so the "need to go to the API documentation" argument doesn't really look valid to me.

@akudev, Yeah, I thought that it might be the answer, I do understand the logic behind.

Regarding the Input$Change and InputBase$Change I do agree that it is pretty easy to find the correct base class via editor completion items for sap.m.Input, however, it's not always so transparent and simple. Couple of examples:

1) sap.m.Table -> selectionChange is borrowed from sap.m.ListBase, which is not so obvious, I would expect e.g. sap.m.TableBase, so not so easy to find via completion items. 2) sap.m.Select -> change event. If sap.m.Input -> change comes from sap.m.InputBase, first guess is that sap.m.Select -> change comes from sap.m.InputBase as well, but it is not the case, because sap.m.Select extends sap.ui.core.Control. Afterwards you understand that maybe it's because there is no way to enter a value, that is why it is not an InputBase. 3) sap.m.ComboBox -> change event. After making notes from previous point No. 2, it would make sense, that ComboBox extends sap.m.InputBase, because it has a way to input a value, and it does extend InputBase, but it has its own ComboBox$ChangeEvent, and it should be used instead of InputBase$ChangeEvent.

Short summary: sap.m.Table#selectionChange -> ListBase$SelectionChangeEvent sap.m.Select#change -> Select$ChangeEvent sap.m.Input#change -> InputBase$ChangeEvent sap.m.ComboBox#change -> ComboBox$ChangeEvent sap.m.TextArea#change -> InputBase$ChangeEvent, but sap.m.TextArea#liveChange -> TextArea$LiveChangeEvent

So yeah, I would not agree that it is very fast and easy to find the correct event type without going to the API.

Without checking the API (or memorizing all parents):

you never know if the control has its own event type (e.g. as sap.m.Select#change)
you never know the name of the parent (e.g. sap.m.Select#change extends Control, but ComboBox extends ComboBoxBase, which extends ComboBoxTextField, which extends InputBase),
you can never "generate" parent name in your head. (E.g. sap.m.Table parent is not sap.m.TableBase, sap.m.TextArea parent is not sap.m.TextAreaBase, sap.m.DatePicker first parent is not sap.m.DatePickerBase and not even sap.m.InputBase, but sap.m.DateTimeField)

I've created a linter for such purposes, it helps to avoid event type mistakes, but without the linter I would say that there will be pretty high probability of misusage of the event types in the future.

Thanks for considering the idea though, I would not forgive myself if I didn't try to suggest it :)

iljapostnovs commented 1 year ago

@akudev, Just figured out third argument in favor of my idea: easier support during UI5 Lib upgrades.

Let's take this example: sap.m.TextArea#change -> InputBase$ChangeEvent sap.m.TextArea#liveChange -> TextArea$LiveChangeEvent

Let's assume that in future UI5 releases there will be introduced change event for sap.m.TextArea for whatever reasons, e.g. new parameter will be introduced. That means, that all previously used types should be replaced from InputBase$ChangeEvent to TextArea$Change in existing codebase. That will not happen if my idea gets implemented, because TextArea$Change will be in the old codebase already :)

I am not trying to argue with your opinion, just providing some things to consider in the future.

akudev commented 1 year ago

That means, that all previously used types should be replaced from InputBase$ChangeEvent to TextArea$Change in existing codebase.

No, not necessarily. Yes, TextArea$Change might get new parameters, but the existing codebase does not use them, so there is no need to change all occurrences. (And I think it's unlikely that many of the occurrences, which all worked just fine so far, now suddenly can't live without that new parameter.)

I do get your point and see its value, but we have to balance cost and benefit. We already had the dtslint run over at DefinitelyTyped crash being out-of-memory and editor performance doesn't get better with a few thousand more types being added "just in case".

This is not a rejection at this point, just thoughts.

akudev commented 1 year ago

To sweeten your disappointment: the original request is going to be implemented with the 1.117 types (supported by https://github.com/SAP/openui5/commit/ee5eb7fc712830a12e03c7dd651ce1660f0bbea3). ;-)

iljapostnovs commented 1 year ago

To sweeten your disappointment: the original request is going to be implemented with the 1.117 types (supported by SAP/openui5@ee5eb7f). ;-)

It's great news :) Thank you!

iljapostnovs commented 1 year ago

@akudev, I believe that this issue can be closed

akudev commented 1 year ago

Alright. :-) The original request is implemented since the 1.117 types and the advanced one is not likely to be. Thanks for the input!

iljapostnovs commented 1 year ago

Thank you! :)

SAP / ui5-typescript

Feature request: adjust getSource method return type for sap.ui.base.Event #401