SAP / ui5-typescript

Tooling to enable TypeScript support in SAPUI5/OpenUI5 projects
https://sap.github.io/ui5-typescript
Apache License 2.0
200 stars 28 forks source link

Type issue with `1.115` upgrade #416

Open dfenerski opened 1 year ago

dfenerski commented 1 year ago

Following type upgrade, now code has to look like this: let a = MessageBox.Action.ABORT;

However when having to type out an argument of a callback function with the possible Actions, I find myself having to do this:

                    myCallback(
                        action: (typeof MessageBox.Action)[keyof typeof MessageBox.Action],
                    ) => {
                        if (action === MessageBox.Action.OK) {
                            allGood()
                        } else {
                            badStuff();
                        }
                    }

which is quite cumbersome. The natural approach of typing action as MessageBox.Action does not seem to work, I get

Cannot access 'MessageBox.Action' because 'MessageBox' is a type, but not a namespace. Did you mean to retrieve the type of the property 'Action' in 'MessageBox' with 'MessageBox["Action"]'

Am I missing something?

Many thanks

akudev commented 11 months ago

Does

action: keyof typeof MessageBox.Action

in the second line work for you to have at least some simplification? I think it should, as the keys (alternative proposal) equal the values (your code) in UI5 enums.

(Plus, of course there is the possibility to say MessageBox["Action"] as suggested in the error message.)

Still not perfect, I know. We'll check.

dfenerski commented 11 months ago

Thanks for the hint, I've missed that one! It works and not having to type 2 sets of brackets is a big win.

dfenerski commented 10 months ago

Hey @akudev, I noticed another thing I wanted to ask about. Tell me if I it does not belong here - It's not directly related to this issue.

During development, I've noticed the JQuery.Event is not generic: image

This forces type casting when retrieving the "parameters" of the event - in this case the theme. It's not real parameters, its assigned to the object itself and also its not real event, its a jQuery one & with future removal intent of the lib IDK if it's worth a discussion.

gssales commented 9 months ago

Hello everyone, I just upgraded to this package and I'm having the same issue to import the Action enum from the MessageBox. I know that in older packages of the type declarations the enums were exported with the controls, is there a reason why this was changed for this package?

akudev commented 9 months ago

@dfenerski

JQuery.Event is not generic

True, but it's defined by jQuery, not by us. We could probably extend it, but yeah, not really investing into jQuery anymore.

Plus, I wonder how you got to use JQuery.Event in this place. Your onThemeChanged is a handler registered via the Core's attachThemeChanged? But it passes a sap.ui.base.Event, not a jQuery one.
You could reply: "Ha! Why is it not generic, then?". Well, the reason why this event (and IIRC half a dozen others) are not generic is because how we did it was generically making them generic in the dts generator, but this covered only events fired by classes inheriting from EventProvider. Core isn't one, but does the eventing manually.

We still could make it generic, but attachThemeChanged is deprecated now and its replacement Theming.attachApplied(...) does have a typed event: image

Again, Theming is not an event provider, hence the typing was done manually and it seems the parameters are directly on the event instead of accessible via getParameter.

akudev commented 9 months ago

@gssales It was done because this fits the UI5 runtime. Previously, dynamic imports like import("sap/m/MessageBox").Action seemed to work in TypeScript, but did not work with the actual UI5 runtime after transformation because only .default is defined.