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

Allow generic type definitions & non-default exports #357

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions packages/ts-interface-generator/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,18 +97,78 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript's

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence has been taken over into #476


Unfortunately these separate interface declarations cannot define new constructors (see e.g. [this related TS issue](https://github.com/microsoft/TypeScript/issues/2957)). Hence those must be manually added to each control (one-time effort, pasting 3 lines of code). The interface generator writes the required lines to the console.

Oh, and the tool itself is implemented in TypeScript because TypeScript makes development more efficient. ;-)

## Features

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


- **Named export:**

```typescript
export class MyCustomControl extends Control {
...
}
```

becomes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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)


```typescript
export interface MyCustomControl {
...
}
```

- **Default export:**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


```typescript
export default abstract class MyAbstractControl extends Control {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

...
}
```

becomes

```typescript
export default interface MyAbstractControl {
...
}
```

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

It takes care that types used to constrain the generic type parameter are imported in the generated interface if required.

```typescript
export interface ICommonListOptions {
mode: 'ul' | 'ol';
}

export default abstract class MyList<TListOptions extends ICommonListOptions> {
...
}
```

becomes

```typescript
export default interface MyList<TListOptions extends ICommonListOptions> {
...
}
```

## TODO

- copy the original API documentation to the generated methods
- make sure watch mode does it right (also run on deletion? Delete interfaces before-creating? Only create interfaces for updated files?)
- consider further information like deprecation etc.
- it is probably required to check whether the control class being handled is the default export or a named export. Right now it is assumed that it is the default export. Other cases are not tested and likely not working.
- ...

## Support
Expand Down
121 changes: 121 additions & 0 deletions packages/ts-interface-generator/src/astGenerationHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,126 @@ function createBindingStringTypeNode() {
);
}

function generateGenericTypeImports(
sourceFile: ts.SourceFile,
classDeclaration: ts.ClassDeclaration,
statements: ts.Statement[],
requiredImports: RequiredImports
): ts.Statement[] {
const { typeParameters } = classDeclaration;
const requiredGenericTypeImports: ts.Statement[] = [];

if (!typeParameters || typeParameters?.length === 0) {
return requiredGenericTypeImports;
}

const existingImportsInSourceFile: {
[name: string]: { statement: ts.ImportDeclaration; exportName?: string };
} = {};

for (const statement of sourceFile.statements) {
if (ts.isImportDeclaration(statement)) {
if (statement.importClause) {
const { name, namedBindings } = statement.importClause;

if (name) {
existingImportsInSourceFile[name.getText()] = { statement };
}

if (namedBindings) {
namedBindings.forEachChild((node) => {
if (ts.isImportSpecifier(node)) {
const typeName = node.name.getText();
let exportName = typeName;

if (node.propertyName) {
exportName = node.propertyName.getText();
}

existingImportsInSourceFile[typeName] = { statement, exportName };
}
});
}
}
}
}

for (const typeParameter of typeParameters) {
if (typeParameter.constraint) {
const typeName = typeParameter.constraint.getText();

if (nameIsUsed(typeName, requiredImports)) {
// import is already created
continue;
} else if (
Object.prototype.hasOwnProperty.call(
existingImportsInSourceFile,
typeName
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) {
const { statement, exportName } = existingImportsInSourceFile[typeName];

const moduleName = statement.moduleSpecifier.getText();
const moduleSpecifierClone = factory.createStringLiteral(
moduleName.substring(1, moduleName.length - 1)
);

let importClause: ts.ImportClause;
const typeNameIdentifier = factory.createIdentifier(typeName);

if (!exportName) {
importClause = factory.createImportClause(
true,
typeNameIdentifier,
undefined
);
} else {
const propertyName =
typeName !== exportName
? factory.createIdentifier(exportName)
: undefined;

let importSpecifier: ts.ImportSpecifier;

// TODO: Use a method to check for versions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (parseFloat(ts.version) >= 4.5) {
// @ts-ignore after 4.5, createImportSpecifier got a third parameter (in the beginning!). This code shall work with older and newer versions, but as the compile-time error check is considering either <4.5 or >=4.5, one of these lines is recognized as error
importSpecifier = factory.createImportSpecifier(
true,
propertyName,
typeNameIdentifier
);
} else {
// @ts-ignore after 4.5, createImportSpecifier got a third parameter (in the beginning!). This code shall work with older and newer versions, but as the compile-time error check is considering either <4.5 or >=4.5, one of these lines is recognized as error
importSpecifier = factory.createImportSpecifier(
propertyName,
typeNameIdentifier
);
}

importClause = factory.createImportClause(
false,
undefined,
factory.createNamedImports([importSpecifier])
);
}

const clone = factory.createImportDeclaration(
statement.decorators,
statement.modifiers,
importClause,
moduleSpecifierClone,
statement.assertClause
);

requiredGenericTypeImports.push(clone);
}
}
}

return requiredGenericTypeImports;
}

function printConstructorBlockWarning(
settingsTypeName: string,
className: string,
Expand Down Expand Up @@ -1231,6 +1351,7 @@ function createConstructorBlock(settingsTypeName: string) {
export {
generateMethods,
generateSettingsInterface,
generateGenericTypeImports,
addLineBreakBefore,
createConstructorBlock,
};
33 changes: 26 additions & 7 deletions packages/ts-interface-generator/src/interfaceGenerationHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
generateMethods,
generateSettingsInterface,
addLineBreakBefore,
generateGenericTypeImports,
} from "./astGenerationHelper";
import astToString from "./astToString";
import log from "loglevel";
Expand Down Expand Up @@ -34,6 +35,8 @@ const interestingBaseSettingsClasses: {
'"sap/ui/core/Control".$ControlSettings': "$ControlSettings",
};

const interfaceIncompatibleModifiers = new Set([ts.SyntaxKind.AbstractKeyword]);

/**
* Checks the given source file for any classes derived from sap.ui.base.ManagedObject and generates for each one an interface file next to the source file
* with the name <className>.gen.d.ts
Expand Down Expand Up @@ -429,13 +432,15 @@ function generateInterface(
{
sourceFile,
className,
classDeclaration,
settingsTypeFullName,
interestingBaseClass,
constructorSignaturesAvailable,
metadata,
}: {
sourceFile: ts.SourceFile;
className: string;
classDeclaration: ts.ClassDeclaration;
settingsTypeFullName: string;
interestingBaseClass: "ManagedObject" | "Element" | "Control" | undefined;
constructorSignaturesAvailable: boolean;
Expand Down Expand Up @@ -478,7 +483,8 @@ function generateInterface(
const moduleName = path.basename(fileName, path.extname(fileName));
const ast = buildAST(
classInfo,
sourceFile.fileName,
sourceFile,
classDeclaration,
constructorSignaturesAvailable,
moduleName,
settingsTypeFullName,
Expand All @@ -494,12 +500,15 @@ function generateInterface(

function buildAST(
classInfo: ClassInfo,
classFileName: string,
sourceFile: ts.SourceFile,
classDeclaration: ts.ClassDeclaration,
constructorSignaturesAvailable: boolean,
moduleName: string,
settingsTypeFullName: string,
allKnownGlobals: GlobalToModuleMapping
) {
const { fileName: classFileName } = sourceFile;

const requiredImports: RequiredImports = {};
const methods = generateMethods(classInfo, requiredImports, allKnownGlobals);
if (methods.length === 0) {
Expand All @@ -518,14 +527,24 @@ function buildAST(

const statements: ts.Statement[] = getImports(requiredImports);

const requiredGenericTypeImports = generateGenericTypeImports(
sourceFile,
classDeclaration,
statements,
requiredImports
);

if (requiredGenericTypeImports.length > 0) {
statements.push(...requiredGenericTypeImports);
}

const myInterface = factory.createInterfaceDeclaration(
undefined,
[
factory.createModifier(ts.SyntaxKind.ExportKeyword),
factory.createModifier(ts.SyntaxKind.DefaultKeyword),
],
classDeclaration.modifiers?.filter(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this does not cover cases like

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

(but I covered both in #476)

(modifier) => !interfaceIncompatibleModifiers.has(modifier.kind)
),
classInfo.name,
undefined,
classDeclaration.typeParameters,
undefined,
methods
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import ts from "typescript";
import { generateInterfaces } from "../interfaceGenerationHelper";
import { initialize } from "../typeScriptEnvironment";

test("Generating the interface for a class using generics", () => {
const expected = `import { PropertyBindingInfo } from "sap/ui/base/ManagedObject";
import { $ManagedObjectSettings } from "sap/ui/base/ManagedObject";

declare module "./SampleGenericManagedObject" {

/**
* Interface defining the settings object used in constructor calls
*/
interface $SampleGenericManagedObjectSettings extends $ManagedObjectSettings {
text?: string | PropertyBindingInfo;
}

export default interface SampleGenericManagedObject<TOptions, TOptions2> {

// property: text
getText(): string;
setText(text: string): this;
}
}
`;

function onTSProgramUpdate(
program: ts.Program,
typeChecker: ts.TypeChecker,
changedFiles: string[], // is an empty array in non-watch case; is at least one file in watch case
allKnownGlobals: {
[key: string]: { moduleName: string; exportName?: string };
}
) {
// files recognized as "real" app source files should be exactly one: SampleGenericManagedObject.ts
const sourceFiles: ts.SourceFile[] = program
.getSourceFiles()
.filter((sourceFile) => {
if (
sourceFile.fileName.indexOf("@types") === -1 &&
sourceFile.fileName.indexOf("node_modules/") === -1 &&
sourceFile.fileName.indexOf(".gen.d.ts") === -1
) {
return true;
}
});
expect(sourceFiles).toHaveLength(1);
expect(sourceFiles[0].fileName).toMatch(/.*SampleGenericManagedObject.ts/);

// this function will be called with the resulting generated interface text - here the big result check occurs
function checkResult(
sourceFileName: string,
className: string,
interfaceText: string
) {
expect(sourceFileName).toMatch(/.*SampleGenericManagedObject.ts/);
expect(className).toEqual("SampleGenericManagedObject");

expect(interfaceText).toEqual(expected);
}

// trigger the interface generation - the result will be given to and checked in the function above
generateInterfaces(
sourceFiles[0],
typeChecker,
allKnownGlobals,
checkResult
);
}

initialize("./tsconfig-testgenerics.json", onTSProgramUpdate, {});
});
Loading