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

typescript generics POC #5

Closed
wants to merge 3 commits into from

Conversation

jkoenig134
Copy link

Hey there,

this PR is a POC for my proposal for a better API for ui5 with typescript.

I scimmed the project and my eyes often stucked on as X type-casts. The more typescript way is using a generic method for giving the user the possibility of defining what the method will return.

This will make calls like

(this.byId("submitButton") as Button).setText(this.oBundle.getText("updateButtonText"));

can be written as

this.byId<Button>("submitButton").setText(this.oBundle.getText("updateButtonText"));

This looks cleaner, is more readable and the programmer can get rid of the brackets around the typecast.
It also will depict the ui5 api in a better way, because of it IS indeed generic in these types of functions I touched in this PR.

@vobu 🎉

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

@akudev
Copy link
Contributor

akudev commented Apr 9, 2021

@jkoenig134 Thanks a lot for this suggestion - that's exactly the kind of input we are looking at right now.
The *.d.ts files are generated, so we'll have to do the actual change inside the generator (not in a public repo right now, we are turning the old one upside down). If I merge this pull request right away, the sample will look better, but if we then update the d.ts files before the generator spits out the generics, the app code will break. So I'll leave it open for the moment - it's anyway meant as a POC - but we'll definitely look into generating better *.d.ts files along these lines.

@jkoenig134
Copy link
Author

@akudev yes.. I was aware that the PR is not mergeable.
Thanks for providing a feedback that fast.. I am really exited what will be the outcome!

@akudev
Copy link
Contributor

akudev commented Apr 12, 2021

Further details from an internal discussion:

First, the confirmation: this is definitely something we want to provide.

There was already a similar list of important methods (plus View.byId()) we had in mind internally. But instead of hardcoding it, we plan to provide a custom JSDoc tag (the *.d.ts files are generated from JSDoc) where such methods can be marked that benefit from generics. So it could take a short while. In theory we could find out automatically when an API returns something that has known subclasses, but this might result in an over-verbose and complicated API, so we'd likely focus on the important ones.

There is an issue about the proposal in the PR: when you write <T = Component>, the receiving variable can in fact have ANY type. "Component" is only the default, but TypeScript will accept the conversion to any type, even if it is not a sub-class of Component. Hence, we think would be more appropriate, no?

I'll leave the PR open for now - for the discussion and as reminder/tracker for the feature.

Thanks again!

@jkoenig134
Copy link
Author

jkoenig134 commented Apr 14, 2021

@akudev the any type is not the problem imo. You could also use (this.byId("submitButton") as any).setText(this.oBundle.getText("updateButtonText"));

I found another way, to make sure, that you can't throw in wrong classes - e.g. not extending UI5Element

byId<T extends UI5Element = UI5Element>(
      /**
       * View-local ID
       */
      sId: string
): T;

By making T extending UI5Element you make sure that there will only classes be thrown in, that extend UI5Element.
I really think that you can't do anything against any casting..

edit: I updated the PR with this proposal 👍🏼

@akudev
Copy link
Contributor

akudev commented May 17, 2021

@jkoenig134 Re-reading my comment: "Hence, we think would be more appropriate, no?" there is clearly something missing in the middle after "think". :-)
I guess it was a copy&paste failure I also wanted to suggest using "extends".
As I wrote: this is definitely planned, there is just some machinery needed in the background, so it's going to take a little longer than just editing the d.ts files.

@codeworrior
Copy link

Sigh. It seems that this proposal conflicts with the dtslint rule no-unnecessary-generics.

Microsoft names this use of generics a 'disguised type assertion' and rather suggests to use as T.

I'm still in favour of the idea, but wanted to get some opinions on that conflict.

@jkoenig134 jkoenig134 closed this Dec 16, 2021
@akudev
Copy link
Contributor

akudev commented Mar 7, 2022

Reference to the "disguised type assertion" statement from Microsoft: https://github.com/DefinitelyTyped/DefinitelyTyped#common-mistakes

if a type parameter does not appear in the types of any parameters, you don't really have a generic function, you just have a disguised type assertion. Prefer to use a real type assertion

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

Successfully merging this pull request may close these issues.

None yet

4 participants