ui5-typescript icon indicating copy to clipboard operation
ui5-typescript copied to clipboard

Allow generic type definitions & non-default exports

Open danielang opened this issue 3 years ago • 4 comments
trafficstars

feat(ts-interface-generator): handeling generic type definitions fix(ts-interface-generator): handling for non-default export ManagedObjects

danielang avatar May 25 '22 13:05 danielang

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

:white_check_mark: danielang
:x: Daniel Lang


Daniel Lang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

cla-assistant[bot] avatar May 25 '22 13:05 cla-assistant[bot]

@danielang Thanks a lot for your contribution! Would you mind also providing a piece of documentation explaining what this allows and what typical use-cases would be? I'm not yet sure about the best place for it, maybe in the Readme here or in the control development or control library sample. Better here, I guess. maybe in a new "Features" section.

akudev avatar May 30 '22 17:05 akudev

Hey @akudev, sure 🙂 I added a section in the readme file of this package. I hope it goes in the right direction. I would be happy to receive some feedback. The changes in this PR allows to use the ts-interface-generator also with named exports. It also allows to make use of TypeScripts generic type parameters, optionally with type constrains.

danielang avatar May 31 '22 16:05 danielang

@danielang While the code is fine and the docu just has minor issues, I think there is a problem in how it works: when the generic class is used in its own API (and probably also when the class is used in APIs of other controls), then the generated methods in the interface do not have the generics part.

Example (shortened):

export default class SampleControl<T extends Toolbar> extends Control {
	static readonly metadata = {
		aggregations: {
			partner: { type: "SampleControl" }
		},
	};
}

causes the generation of

declare module "./SampleControl" {

    interface $SampleControlSettings extends $ControlSettings {
        partner?: SampleControl[] | SampleControl | AggregationBindingInfo | `{${string}}`;
    }

    export default interface SampleControl<T extends Toolbar> {
        // aggregation: partner
        getPartner(): SampleControl[];
        addPartner(partner: SampleControl): this;
        insertPartner(partner: SampleControl, index: number): this;
        removePartner(partner: number | string | SampleControl): this;
        removeAllPartner(): SampleControl[];
        indexOfPartner(partner: SampleControl): number;
        destroyPartner(): this;
    }
}

There's the class name "SampleControl" all over the place - without generics. Which makes TS complain: "Generic type 'SampleControl<T>' requires 1 type argument(s).".

One could argue that the metadata actually it should be:

			partner: { type: "SampleControl<T extends Toolbar>" }

but this doesn't work either because

  1. the interface generator treats this string as a plain class name and tries to import the whole thing (fixable within the generator) and
  2. this string will still be there at runtime - and not be understood by UI5 (only fixable inside the babel transformer plugin, but that's the wrong place, as it shouldn't handle TypeScript input)

Right now I don't know how to deal with this. Of course writing types as strings in the metadata section is ~~legacy~~ not helping here, but it is what we have now and changing it would be a major development effort. We could also simply state that using generic classes in the control API definition is not supported.

What do you think? And you, @codeworrior , @petermuessig ?

akudev avatar Jun 03 '22 16:06 akudev