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

Type issue with 1.115 upgrade #416

Open
dfenerski opened this issue Sep 15, 2023 · 6 comments
Open

Type issue with 1.115 upgrade #416

dfenerski opened this issue Sep 15, 2023 · 6 comments

Comments

@dfenerski
Copy link

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
Copy link
Contributor

akudev commented Oct 6, 2023

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
Copy link
Author

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
Copy link
Author

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
Copy link

gssales commented Nov 30, 2023

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
Copy link
Contributor

akudev commented Dec 4, 2023

@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
Copy link
Contributor

akudev commented Dec 4, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants