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

Missing documentation for event type names in SDK #415

Open
EricVanEldik opened this issue Sep 1, 2023 · 5 comments
Open

Missing documentation for event type names in SDK #415

EricVanEldik opened this issue Sep 1, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@EricVanEldik
Copy link

Hi,

Ever since 1.115 my team is having issues setting the correct type on events, There seems to be no up-to-date documentation or clear Types indaction;
For example before 1.115, this was correct:

XML:

<dragDropConfig>
     <dnd:DropInfo drop="onDropFile"/>
</dragDropConfig>

Controller

  onDropFile(event: Event) {
          const browserEvent = event.getParameter("browserEvent") as DragEvent;
          if (browserEvent.dataTransfer.files) {
              const files = browserEvent.dataTransfer.files;
              this.attachFiles(files);
          }
      }

After 1.115 this is no longer correct, but according to the documentation this is correct: https://ui5.sap.com/1.116.0/#/api/sap.ui.core.dnd.DropInfo%23events/drop

If we try to do it the suggested way according to the release notes we get ( I think this:
Controller

   onDropFile(event: DropInfo$DropEvent) {
        const browserEvent = event.getParameter("browserEvent") as DragEvent;
        if (browserEvent.dataTransfer.files) {
            const files = browserEvent.dataTransfer.files;
            this.attachFiles(files);
        }
    }

However there isn't any combination that works with DragEvent, and without documentation this just becomes a guesing game.

Is there something we are missing or is the documentation just not up-to-date?

@akudev
Copy link
Contributor

akudev commented Sep 4, 2023

Hi Eric,

There are several aspects to this ticket and I'll try to address all of them:

1.) Just to explain the overall motivation (even though it might be clear to you already, anyway): event.getParameter("xyz") is no longer correct when event is simply typed as sap.ui.base.Event. Yes, this is true and intended and a good thing that is in line with what you expect from TypeScript, even though - as often in TypeScript - it forces you to write a bit more or express things in a more precise way. Being able to put in any string without any check and then getting something back that is typed as any was a typing gap in the UI5 types. Now each event type knows which parameters it has and how they are typed. This means you get the existing parameter names as suggestions and won't accidentally mis-type them or use parameters that don't exist. Plus the result already has the correct type. This is exactly what TypeScript is used for. And as the base type Event does not have parameter "xyz", TypeScript will let you know that accessing it is wrong. So this new behavior is definitely intended.

2.) The following code does not work and this is a bug:

 onDropFile(event: DropInfo$DropEvent) {
      const browserEvent = event.getParameter("browserEvent") as DragEvent;

The "browserEvent" parameter is typed as UI5 Event, hence TypeScript complains about the cast to the browser's DragEvent (because "the types do not sufficiently overlap"). The parameter should rather already be typed as DragEvent, which would make the typecast in the application code unneeded. I'm not sure whether this is a bug in the documentation of the DropInfo's Drop event or in the type generation: the DropInfo indeed documents it as Event, not sap.ui.base.Event, and in the code editor it is understood as browser event. Somewhere along the way it is turned into a UI5 event, though. We need to investigate and fix, but for the time being you can write:

 onDropFile(event: DropInfo$DropEvent) {
      const browserEvent = event.getParameter("browserEvent") as unknown as DragEvent;

With this, the subsequent access to the dataTransfer property will work just fine.

3.) For the rest I'm not totally clear what you mean: "[no] combination that works with DragEvent, and without documentation"...
Do you refer to the not-working cast to DragEvent I explained above? Or to another issue related to a Drag* event of DropInfo or DragInfo? What exactly would be the "missing documentation"? Is this complaint related to the composed names of the events not being in the UI5 API documentation or to something else?

@akudev akudev added the bug Something isn't working label Sep 4, 2023
@EricVanEldik
Copy link
Author

Thank you @akudev for the explination, for point 3: I was refering to problem above and the documentation in https://ui5.sap.com/1.116.0/#/api/sap.ui.core.dnd.DropInfo%23events/drop not matching the type indication.

I understand what you try to achieve with the changes in 1.115, but without proper documentation a piece of code like onDescriptionSearch(event: SearchField$LiveChangeEvent) { is hard to find the proper type, since there is no refference in the documenation to LiveChangeEvent being the type that is outputed when using livechange.

@akudev
Copy link
Contributor

akudev commented Sep 4, 2023

Ok,... for problem 2.) a fix is on its way already.

The more general problem, to rephrase it, is that the type names for the events are not documented anywhere, making it hard to know them, right?

On the one hand we hoped that the consistent <ControlName>$<EventName>Event way how the event type names are constructed would make it easy to come up with them. E.g. for https://ui5.sap.com/1.116.0/#/api/sap.ui.core.dnd.DropInfo%23events/drop it would be the control/class name first ("DropInfo"), then a dollar sign, then the event name ("Drop") and then the constant string "Event", which makes it DropInfo$DropEvent.

On the other hand I see why it would be better to simply have it explicitly written in the documentation.

The challenges for this are:

  • In the build pipeline those event type names do not exist at all at the stage where the documentation is generated, they are only created by the dts generator for TypeScript.
  • The event names are not relevant for JavaScript usage and introducing hundreds of new pages in the API documentation leads to additional clicking effort for JS users to get to the relevant definition of the sap.ui.base.Event.
  • Completely changing how events are documented also seems like significant effort for the code of the SDK's API documentation.
    These challenges don't make the request invalid - it totally makes sense for better TypeScript support - , but harder to implement, hence this is not a thing that will happen quickly.

Where it currently says oControlEvent [sap.ui.base.Event](https://ui5.sap.com/1.116.0/api/sap.ui.base.Event) it should probably say DropInfo$DropEvent. But then the "getSource" and the parameters listed below should rather go to the other linked page where DropInfo$DropEvent is defined. This would make usage harder for everyone. Maybe a short-term alternative would be to use the "Description" column, which is so far unused, to mention the TypeScript type for the event, without a link to a dedicated page for this type? The effort would be limited if we just apply a copy of the event name generation algorithm locally.

@akudev akudev changed the title Missing documentation Missing documentation for event type names in SDK Sep 4, 2023
@EricVanEldik
Copy link
Author

A reference in the documentation would be a plus, however I think you explination with example allready clear enough (for me).
It was just bad luck that the second method I tried to correct was the one that was bugged.

openui5bot pushed a commit to SAP/openui5 that referenced this issue Sep 5, 2023
"DragEvent" is more precise on the one hand, leading to a better
TypeScript experience, and on the other hand avoids the issue that
simply "Event" is somehow (wrongly) interpreted as UI5 event (should be
a browser event).

Related to SAP/ui5-typescript#415

Change-Id: I7be6df5ab541bc8a5ec47eb48cbbacbb60f115fe
@akudev
Copy link
Contributor

akudev commented Sep 5, 2023

Bug is fixed with SAP/openui5@43e032c for the 1.119 types.

The general event name issue is still open.

@akudev akudev added enhancement New feature or request and removed bug Something isn't working labels Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants