Allow generic type definitions & non-default exports #357

danielang · 2022-05-25T13:35:01Z

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

fix(ts-interface-generator): handling for non-default export ManagedObjects

cla-assistant · 2022-05-25T13:35:26Z

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.

✅ danielang
❌ danielang-ortec
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

akudev · 2022-05-30T17:23:54Z

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

danielang · 2022-05-31T16:05:56Z

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.

akudev

Thanks, this looks good. Just some minor things grammar/typo nits. I'll have another look at the code later.

akudev · 2022-06-01T08:45:17Z

packages/ts-interface-generator/README.md

@@ -95,18 +95,75 @@ This is a problem for application code using controls developed in TypeScript as

 This tool scans all TypeScript source files for <b>top-level definitions of classes</b> inheriting from `sap.ui.base.ManagedObject` (in most cases those might be sub-classes of `sap.ui.core.Control`, so they will be called "controls" for simplicity).

-For any such control, the metadata is parsed, analyzed, and a new TypeScript file is constructed, which contains an interface declaration with the methods generated by UI5 at runtime.
+For any such control, the metadata is parsed, analyzed, and a new TypeScript file is constructed, which contains an interface declaration with the methods generated by UI5 at runtime. This generated interface gets merged with the already existing code using TypeScripts [Declaration Merging](https://www.typescriptlang.org/docs/handbook/declaration-merging.html) concept.


TypeScript's

This sentence has been taken over into #476

akudev · 2022-06-01T08:47:04Z

packages/ts-interface-generator/README.md

+
+### Handling `default` and named exports
+
+In order that the Declaration Merging of TypeScript works, the modifiers of the generated interface has to match the parsed UI5 artifact.


Plural or singular? Decide for either "modifiers ... have to" or "modifier ... has to".

akudev · 2022-06-01T08:49:11Z

packages/ts-interface-generator/README.md

+  }
+  ```
+
+- **Default export:**


As regular UI5 controls are default exports of their module and custom control owners might strive for similar usage, I would mention this case first - before the named exports. (possibly also mention that standard UI5 controls are done like this)

akudev · 2022-06-01T08:52:35Z

packages/ts-interface-generator/README.md

+
+- **Default export:**
+  ```typescript
+  export default abstract class MyAbstractControl extends Control {


Is there a particular reason for this control to be named "...Abstract..."? At first glance I would have expected both code samples to be as similar as possible apart from the difference which is supposed to be demonstrated.

akudev · 2022-06-01T08:54:18Z

packages/ts-interface-generator/README.md

+  }
+  ```
+
+  becomes


"becomes" sounds like a transformation to me. What about being a bit more verbose and saying "for ... the following interface is generated" or so?
(same in the other samples)

akudev · 2022-06-01T08:56:15Z

packages/ts-interface-generator/README.md

+
+### Generics
+
+This tool also supports generic classes which extending `ManagedObject` class. In order to enable the Declaration Merging of TypeScript the generic type parameters needs to be incooperated into the generated interface declaration.


"which are extending" or I think better: "which extend" (no native speaker here, but we are not describing a current action, but rather a general state)

akudev · 2022-06-01T08:57:58Z

packages/ts-interface-generator/README.md

+
+### Generics
+
+This tool also supports generic classes which extending `ManagedObject` class. In order to enable the Declaration Merging of TypeScript the generic type parameters needs to be incooperated into the generated interface declaration.


Plural: "parameters need to be"
And:
I guess it's spelled "incorporated"

akudev

Code looks good to me as well, I'll try out one or two things with the changed code.

akudev · 2022-06-03T14:25:11Z

packages/ts-interface-generator/src/astGenerationHelper.ts

+        Object.prototype.hasOwnProperty.call(
+          existingImportsInSourceFile,
+          typeName
+        )


Out of curiosity: how is this different from if (typeName in existingImportsInSourceFile)? Any inherited properties we should be aware of? Don't think so. Or just doing it like this in general to avoid inherited-property-problems in other places?
No need to change, just wondering.

akudev · 2022-06-03T14:32:38Z

packages/ts-interface-generator/src/astGenerationHelper.ts

+
+          let importSpecifier: ts.ImportSpecifier;
+
+          // TODO: Use a method to check for versions


Indeed. :-) But outside the scope of this PR.

akudev · 2022-06-03T16:07:13Z

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

the interface generator treats this string as a plain class name and tries to import the whole thing (fixable within the generator) and
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

In #476 I have implemented, among other things, the "non-default exports" aspect of this change. So at least that part will be supported. As I wrote, there are some issues with the generics, so this remaining part of the PR looks tricky.

akudev · 2024-09-25T10:40:58Z

packages/ts-interface-generator/src/interfaceGenerationHelper.ts

-      factory.createModifier(ts.SyntaxKind.ExportKeyword),
-      factory.createModifier(ts.SyntaxKind.DefaultKeyword),
-    ],
+    classDeclaration.modifiers?.filter(


I think this does not cover cases like

class XYZ { ... } ... export default XYZ;

(but I covered both in #476)

akudev · 2024-09-25T10:52:46Z

packages/ts-interface-generator/README.md

@@ -95,18 +95,75 @@ This is a problem for application code using controls developed in TypeScript as

 This tool scans all TypeScript source files for <b>top-level definitions of classes</b> inheriting from `sap.ui.base.ManagedObject` (in most cases those might be sub-classes of `sap.ui.core.Control`, so they will be called "controls" for simplicity).

-For any such control, the metadata is parsed, analyzed, and a new TypeScript file is constructed, which contains an interface declaration with the methods generated by UI5 at runtime.
+For any such control, the metadata is parsed, analyzed, and a new TypeScript file is constructed, which contains an interface declaration with the methods generated by UI5 at runtime. This generated interface gets merged with the already existing code using TypeScripts [Declaration Merging](https://www.typescriptlang.org/docs/handbook/declaration-merging.html) concept.


This sentence has been taken over into #476

feat(ts-interface-generator): handeling generic type definitions

5239c0d

fix(ts-interface-generator): handling for non-default export ManagedObjects

fix(ts-interface-generator): linting errors of added and modified files

d8e3353

danielang added 2 commits May 31, 2022 17:48

docs(ts-interface-generator): added features section

780ec2c

docs(ts-interface-generator): fix layout of readme

2ffbc8e

docs(ts-interface-generator): fix layout of readme for prettier

4d55970

akudev reviewed Jun 1, 2022

View reviewed changes

akudev reviewed Jun 3, 2022

View reviewed changes

akudev mentioned this pull request Aug 11, 2022

@ui5/ts-interface-generator: add generics to generated interface #376

Open

Merge remote-tracking branch 'sap/main' into feature/generics

cddfe0c

akudev force-pushed the main branch from 570bd17 to 7bc07b5 Compare June 22, 2023 11:36

akudev reviewed Sep 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow generic type definitions & non-default exports #357

Allow generic type definitions & non-default exports #357

danielang commented May 25, 2022

cla-assistant bot commented May 25, 2022 •

edited

Loading

akudev commented May 30, 2022

danielang commented May 31, 2022

akudev left a comment

akudev Jun 1, 2022

akudev Sep 25, 2024

akudev Jun 1, 2022

akudev Jun 1, 2022

akudev Jun 1, 2022

akudev Jun 1, 2022

akudev Jun 1, 2022

akudev Jun 1, 2022

akudev left a comment

akudev Jun 3, 2022

akudev Jun 3, 2022

akudev commented Jun 3, 2022 •

edited

Loading

akudev left a comment

akudev Sep 25, 2024

akudev Sep 25, 2024


		### Handling `default` and named exports

		In order that the Declaration Merging of TypeScript works, the modifiers of the generated interface has to match the parsed UI5 artifact.

+                }
+                ```
+                becomes


		### Generics

		This tool also supports generic classes which extending `ManagedObject` class. In order to enable the Declaration Merging of TypeScript the generic type parameters needs to be incooperated into the generated interface declaration.


		let importSpecifier: ts.ImportSpecifier;

		// TODO: Use a method to check for versions

Allow generic type definitions & non-default exports #357

Are you sure you want to change the base?

Allow generic type definitions & non-default exports #357

Conversation

danielang commented May 25, 2022

cla-assistant bot commented May 25, 2022 • edited Loading

akudev commented May 30, 2022

danielang commented May 31, 2022

akudev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akudev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akudev commented Jun 3, 2022 • edited Loading

akudev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-assistant bot commented May 25, 2022 •

edited

Loading

akudev commented Jun 3, 2022 •

edited

Loading