SAP / ui5-typescript

Tooling to enable TypeScript support in SAPUI5/OpenUI5 projects
https://sap.github.io/ui5-typescript
Apache License 2.0
200 stars 28 forks source link

@ui5/ts-interface-generator: error when when baseclass in located in another package #382

Closed Talwynox closed 1 year ago

Talwynox commented 1 year ago

Describe the bug We have a UI5 Application and a library with a simular structure: -ui5-typescript-helloworld --DerivedClass.ts -ui5-typescript-lib --BaseClass.ts

Where BaseClass.ts looks like this

import Toolbar from 'sap/m/Toolbar';

/**
 * @namespace ui5.typescript.lib
 */
export default class BaseClass extends Toolbar {
  constructor(idOrSettings?: string | $BaseClassSettings);
  constructor(id?: string, settings?: $BaseClassSettings);
  constructor(id?: string, settings?: $BaseClassSettings) {
    super(id, settings);
  }

  static readonly metadata: object = {
    properties: {
      a: { type: 'boolean', defaultValue: true },
    },
  };
}

And DerivedClass.ts looks like this

import BaseClass from "ui5-typescript-lib/src/BaseClass";

/**
 * @namespace ui5.typescript.helloworld.controller
 */
export default class DerivedClass extends BaseClass {
  static readonly metadata = {
    properties: {
      test: { type: "boolean", defaultValue: true },
    },
  };
}

Running the generator in the library work as excepted. However running the generator in the application throws the following error :

Writing interface file: C:\Repo\Test\ui5-typescript\ui5-typescript-lib\src\BaseClass.gen.d.ts

C:\Repo\Test\ui5-typescript\ui5-typescript-helloworld\node_modules\typescript\lib\typescript.js:51691
            return symbol.parent ? getFullyQualifiedName(symbol.parent, containingLocation) + "." + symbolToString(symbol) : symbolToString(symbol, containingLocation, /*meaning*/ undefined, 16 /* SymbolFormatFlags.DoNotIncludeSymbolChain */ | 4 /* SymbolFormatFlags.AllowAnyNodeKind */);
                          ^

TypeError: Cannot read properties of undefined (reading 'parent')
    at Object.getFullyQualifiedName (C:\Repo\Test\ui5-typescript\ui5-typescript-helloworld\node_modules\typescript\lib\typescript.js:51691:27)
    at getInterestingBaseSettingsClass (C:\Repo\Test\ui5-typescript\ui5-typescript-helloworld\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:291:83)
    at getSettingsTypeFromConstructor (C:\Repo\Test\ui5-typescript\ui5-typescript-helloworld\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:259:50)
    at getSettingsType (C:\Repo\Test\ui5-typescript\ui5-typescript-helloworld\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:242:38)
    at C:\Repo\Test\ui5-typescript\ui5-typescript-helloworld\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:121:54
    at Array.forEach (<anonymous>)
    at C:\Repo\Test\ui5-typescript\ui5-typescript-helloworld\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:68:46
    at Array.forEach (<anonymous>)
    at C:\Repo\Test\ui5-typescript\ui5-typescript-helloworld\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:66:43
    at Array.forEach (<anonymous>)

Upon debugging into the generator it looks like the parent of $BaseClassSettings is undefined

Expected behavior Files are generated correctly.

akudev commented 1 year ago

Just a quick question: was this using version 0.5.2 of the @ui5/ts-interface-generator or already the new 0.5.3, which was released yesterday? It does contain a fix related to controls in different folders, but I'm not sure this scenario is covered. Anyway, it would be good if you could try with 0.5.3.

I'm a bit surprised that "running the generator in the application" does write ui5-typescript-lib\src\BaseClass.gen.d.ts anew, but this could be either caused by the project setup including tsconfig.json or by how the generator decides which files to cover.

Talwynox commented 1 year ago

We tested it with both 0.5.2 and 0.5.3, the behavior in this case is identical. As for generator regenerating the file in the library, I have no idea why that happens. If it help the tsconfig.json looks like this

{
  "compilerOptions": {
    "target": "es2015",
    "module": "es2015",
    "moduleResolution": "node",
    "skipLibCheck": true,
    "preserveConstEnums": true,
    "inlineSourceMap": true,
    "allowJs": true,
    "strict": true,
    "strictNullChecks": true,
    "strictPropertyInitialization": false,
    "rootDir": "./src",
    "outDir": "./dist",
    "baseUrl": "./",
    "paths": {
      "ui5/typescript/helloworld/*": ["./src/*"],
      "ui5/typescript/lib/*": ["node_modules/ui5-typescript-lib/src/*"]
    }
  },
  "include": ["./src/**/*"]
}
akudev commented 1 year ago

Ok, thanks, I will have a look and come back to you in case the setup is still not clear enough.

Talwynox commented 1 year ago

Any update on this issue?

akudev commented 1 year ago

Not yet.

akudev commented 1 year ago

FYI: when trying to reproduce this, I first encountered the identical error being thrown when two classes are created at the same time, both already referencing the $...Settings type. In this case, the generator might process them in the wrong order (dependencies are not yet considered) and simply re-trying will solve the problem, as the *.gen.d.ts file for one of the classes is successfully generated then. For the time being I handle this case with additional logging statements to this path, so errors can be understood more easily (hopefully also by the user).

However, I think that in your case the $BaseClassSettings type is not known to the TypeScript compiler as run by the generator and initialized with the current tsconfig of the app. I am now trying to figure out whether changing the tsconfig or changing the generator in some way solves this. On the other hand, the generator, when run inside the app, also re-generates the *.gen.d.ts files of the library inside node_modules, which should clearly not happen.

akudev commented 1 year ago

@Talwynox : I now got to work what I think is your scenario. Could you please try the following:

In tsconfig.json, extend the "include" section to also cover the library:

    "include": [
        "./src/**/*",
        "./node_modules/ui5-typescript-lib/src/**/*"
    ]

At this point, the compiler may complain that the files of this lib are not inside the rootDir. Therefore change it to rootDirs - apparently there is no need to extend the the actual list of dirs:

"rootDirs": ["./src"],

This should already do the job with the version of the generator you currently have, but might wrongly re-generate the BaseClass.gen.d.ts file, which should not happen from the app - the base class is in a used library and should be re-generated only from there. Depending on the code, this can even break the BaseClass.gen.d.ts file. However, I have just released version 0.5.4 of the generator, which solves this cross-generation problem, so I would recommend to update first before trying the above.

Talwynox commented 1 year ago

It is working correctly now, thanks.

akudev commented 1 year ago

But at least in my local scenario it affects the output structure of the TS compiler. It effectively causes the parent of the "webapp" folder I am using for the app to become the rootDir (even though only "webapp" itself is specified as "rootDirs"). And the used library becomes part of the project. This means below "dist" there is "node_modules/..../BaseClass.js" and "webapp/controller/MyController.js" etc., while without the change it was only "controller/MyController.js".

petermuessig commented 1 year ago

I prepared a change for the ui5-tooling-tranpile tooling extension to properly generate the type definitions including the index.d.ts which ensures that e.g. TypeScript UI5 applications consuming UI5 libraries will get code completion by just adding the typeRoots configuration:

Snippet taken from: https://github.com/ui5-community/ui5-ecosystem-showcase/blob/223b9f54745c397f6bf7aec430d45d5a9a33c354/showcases/ui5-tsapp/tsconfig.json:

{
  "compilerOptions": {
    [...]
    "paths": {
      [...]
      "ui5/ecosystem/demo/tslib/*": ["./node_modules/ui5-tslib/src/ui5/ecosystem/demo/tslib/*"]
    },
    "typeRoots": [
      "./node_modules/@types",
      "./node_modules/ui5-tslib"
    ]
  },
  "include": ["./webapp/**/*.ts"]
}

The paths mapping is just included in case of the library hasn't been built yet in the monorepo structure. But this mapping can also be omitted. Important is the addition of the typeRoots configuration - keep in mind, as soon as you add your type root for e.g. node_modules/ui5-tslib, you need to also add the type roots to node_modules/@types so that the types from definitely typed can be found.

Related PR: https://github.com/ui5-community/ui5-ecosystem-showcase/pull/734

petermuessig commented 1 year ago

@Talwynox @akudev during the last week I had a lot of learnings for the handling of TypeScript projects as dependencies inside and outside of monorepos. In short, we need to always generate the type definitions when using the types in other projects so that the code completion works properly. And the linkage of the projects must happen for TypeScript via the types property in the tsconfig.json. So in case of changing APIs in monorepos or when linking (via npm link or file urls), you need to re-run the build to generate the types and the package.json needs the types property pointing to the index.d.ts file. But for functional changes like the behavior or rendering code, no rebuild is needed as this doesn't require an API change and the middleware transpiles on-the-fly.

Important change is the usage of types for custom types and typeRoots must point to ./node_modules/@types (to still include the standard types) and removing the paths mappings over node_modules:

{
  "compilerOptions": {
    [...]
    "paths": {
      [...]
    },
    "typeRoots": [
      "./node_modules/@types",
    ],
    "types": [
      "ui5-tslib"
    ]
  },
  "include": ["./webapp/**/*.ts"]
}

Snippet taken from here: https://github.com/ui5-community/ui5-ecosystem-showcase/blob/main/showcases/ui5-tsapp/tsconfig.json#L17-L18

The library defines the types mapping in the package.json: https://github.com/ui5-community/ui5-ecosystem-showcase/blob/main/showcases/ui5-tslib/package.json#L8

The library build generates the type definitions + declaration maps and links to the sources so that the "Go to Sources Defintion" on a module works in the IDE. In addition, the library build also generates the index.d.ts into the dist folder which must be mapped in the package.json.

@Talwynox provided me his sample project for which I prepared a new PR incorporating all these changes + some more description what, how and why it has been changed: https://github.com/dstigl/UI5-Test-Projects/pull/2

petermuessig commented 1 year ago

@akudev : I think with that we can close this issue here as we have a proper solution now. WDYT?

petermuessig commented 1 year ago

As discussed with @akudev, we close the issue now. The recommendation is to use a mixture of types and typeRoots and using the new feature of the ui5-tooling-transpile to generate the .d.ts of the library and consume this for code completion rather than the source code.

Talwynox commented 1 year ago

@petermuessig Sorry for the late response. We looked at your changes, but now we have the problem, that VS Code can not resolve the imports anymore. Neither the libraries nor the UI5 types. After adding the library paths to compilerOptions>paths-Attribute in the tsconfig file again and "openui5" to the compilerOptions>types-Attribute, the imports can be resolved again. But as you said, this should not be required any more or did we miss something?

Also when we try to build the application we get the following errors:

ERR! builder:customtask:ui5-tooling-transpile The following errors occured during d.ts generation:
ERR! builder:customtask:ui5-tooling-transpile /resources/my/projects/test/control/CustomProjectTextControl.ts(18,39): error TS4063: Parameter 'idOrSettings' of constructor from exported class has or is using private name '$CustomProjectTextControlSettings'.
ERR! builder:customtask:ui5-tooling-transpile /resources/my/projects/test/control/CustomProjectTextControl.ts(19,39): error TS4063: Parameter 'settings' of constructor from exported class has or is using 
private name '$CustomProjectTextControlSettings'.
ERR! builder:customtask:ui5-tooling-transpile /resources/my/projects/test/control/LineChart.ts(13,39): error TS4063: Parameter 'idOrSettings' of constructor from exported class has or is using private name '$LineChartSettings'.
ERR! builder:customtask:ui5-tooling-transpile /resources/my/projects/test/control/LineChart.ts(14,39): error TS4063: Parameter 'settings' of constructor from exported class has or is using private name '$LineChartSettings'.
ERR! builder:customtask:ui5-tooling-transpile

Anyway its possible to run the application from dist folder.

Another error appears when we try to generate the *.d.ts-files using the npm run build:controls-command:

C:\UI5-Test-Projects\Test\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:72
                                throw new Error(`Type '${typeNode.getText()}' referenced in ${sourceFile.fileName} in the inheritance clause '${heritageClause.getFullText()}' could not be resolved.       
                                ^

Error: Type 'UIComponent' referenced in C:/UI5-Test-Projects/Test/webapp/Component.ts in the inheritance clause ' extends UIComponent' could not be resolved.
Check the respective line in the source code: ts there an error for this type? Make sure the type is properly imported.
If a working "import" is not possible and it is a UI5 type (or type from another library), the issue could be caused by the respective type definitions not being available. They must be found by the TypeScript compiler according to the configuration in tsconfig. To verify this step-by-step, you can do the following:
1. Check whether the (UI5 or other) types are added as dependency in package.json (or available as transitive dependency)
2. Check inside which "node_modules" folder the types are actually available - if they are not, check whether "npm install" (or "yarn" etc.) has run successfully - maybe re-run it
3. Check the "tsconfig.json" file: types outside the default "@types" package must be explicitly added in the "types" or "typeRoots" section. Is the name and path correct?
One known cause of this error is that the "typeRoots" setting in tsconfig.json has wrong paths, which are not actually pointing to the correct location of the type definitions.
Or is there a different reason why this type would not be known?
    at C:\UI5-Test-Projects\Test\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:72:39
    at Array.forEach (<anonymous>)
    at C:\UI5-Test-Projects\Test\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:68:46
    at Array.forEach (<anonymous>)
    at C:\UI5-Test-Projects\Test\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:66:43
    at Array.forEach (<anonymous>)
    at getManagedObjects (C:\UI5-Test-Projects\Test\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:62:27)
    at generateInterfaces (C:\UI5-Test-Projects\Test\node_modules\@ui5\ts-interface-generator\dist\interfaceGenerationHelper.js:38:17)
    at C:\UI5-Test-Projects\Test\node_modules\@ui5\ts-interface-generator\dist\generateTSInterfaces.js:134:60
    at Array.forEach (<anonymous>)

Node.js v18.12.1

Any suggestions?