-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
5239c0d
d8e3353
780ec2c
2ffbc8e
4d55970
cddfe0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,18 +95,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. | ||
|
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
|
||
```typescript | ||
export interface MyCustomControl { | ||
... | ||
} | ||
``` | ||
|
||
- **Default export:** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Plural: "parameters need to be" |
||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity: how is this different from |
||
) { | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -1161,6 +1281,7 @@ function createConstructorBlock(settingsTypeName: string) { | |
export { | ||
generateMethods, | ||
generateSettingsInterface, | ||
generateGenericTypeImports, | ||
addLineBreakBefore, | ||
createConstructorBlock, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import { | |
generateMethods, | ||
generateSettingsInterface, | ||
addLineBreakBefore, | ||
generateGenericTypeImports, | ||
} from "./astGenerationHelper"; | ||
import astToString from "./astToString"; | ||
import log from "loglevel"; | ||
|
@@ -43,6 +44,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 | ||
|
@@ -424,13 +427,15 @@ function generateInterface( | |
{ | ||
sourceFile, | ||
className, | ||
classDeclaration, | ||
settingsTypeFullName, | ||
interestingBaseClass, | ||
constructorSignaturesAvailable, | ||
metadata, | ||
}: { | ||
sourceFile: ts.SourceFile; | ||
className: string; | ||
classDeclaration: ts.ClassDeclaration; | ||
settingsTypeFullName: string; | ||
interestingBaseClass: | ||
| "ManagedObject" | ||
|
@@ -479,7 +484,8 @@ function generateInterface( | |
const moduleName = path.basename(fileName, path.extname(fileName)); | ||
const ast = buildAST( | ||
classInfo, | ||
sourceFile.fileName, | ||
sourceFile, | ||
classDeclaration, | ||
constructorSignaturesAvailable, | ||
moduleName, | ||
settingsTypeFullName, | ||
|
@@ -495,12 +501,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) { | ||
|
@@ -519,14 +528,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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
); | ||
|
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, {}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript's
There was a problem hiding this comment.
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