SAP-samples / ui5-cap-event-app

Showcase of SAP Cloud Application Programming Model and OData V4 with draft mode in a freestyle SAPUI5 app and an SAP Fiori elements app.
Apache License 2.0
90 stars 36 forks source link

typescript generics POC #5

Closed jkoenig134 closed 2 years ago

jkoenig134 commented 3 years ago

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 commented 3 years ago

CLA assistant check
All committers have signed the CLA.

akudev commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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.

akudev commented 2 years ago

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