-
Notifications
You must be signed in to change notification settings - Fork 92
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
fix(ui5-tooling-transpile): d.ts files to be modules + generate index.d.ts #734
Conversation
.map( | ||
(dtsFile) => | ||
`/// <reference path=".${ | ||
/\.d\.ts$/.test(dtsFile) ? dtsFile : dtsFile.replace(/\.ts$/, ".d.ts") |
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.
There's always a .d.ts file for each .ts file?
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.
Correct - and for controls there are even 2: the d.ts and the gen.d.ts
"integration/*": ["./webapp/test/integration/*"] | ||
} | ||
"integration/*": ["./webapp/test/integration/*"], | ||
"ui5/ecosystem/demo/tslib/*": ["./node_modules/ui5-tslib/src/ui5/ecosystem/demo/tslib/*"] |
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.
What' the difference here? Why do we still need a path into node_modules?
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.
Or is this related to the following statement from the issue report??
"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. "
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.
The paths mappings are still necessary as long as the library has not been built. Then this is used to lookup the type information from the ts
files. Once built, the type definitions are retrieved from the dist
folder, then the index.d.ts
and the nested d.ts
files are used and the typeRoots
configuration wins.
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.
Looks good to me overall
const source = sourcesMap[resourcePath]; | ||
let moduleName = /^\/resources\/(.*)\.ts$/.exec(resourcePath)?.[1]; | ||
if (moduleName?.endsWith(".gen.d")) { | ||
moduleName = /^(.*)\.gen\.d$/.exec(moduleName)?.[1] || moduleName; |
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.
Ummm... isn't the OR part in the end of this line (and the question mark) unneeded? You already checked that the module name ends with ".gen.d".
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.
Relict from previous logic - will adapt
for await (const resourcePath of Object.keys(sourcesMap)) { | ||
const source = sourcesMap[resourcePath]; | ||
let moduleName = /^\/resources\/(.*)\.ts$/.exec(resourcePath)?.[1]; | ||
if (moduleName?.endsWith(".gen.d")) { |
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.
Hmmm.. so the heuristics is: any module named "*.gen.d" is assumed to be one generated by the ts-interface-generator and hence already containing a module declaration, which has to be modified, though? Should maybe be written down somewhere.
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.
Adding JSDoc
….d.ts This fix ensures the correct d.ts generation. The transpile procees now creates an index.d.ts and all TS UI5 modules will declare its module with the fully qualified name: ```ts declare module "${moduleName}" { ${source} } ``` In addition, the generated control interfaces, the `.gen.d.ts` files need to also be converted to declare the module with the fully qualified name and put the imports into the module declaration.
223b9f5
to
d748335
Compare
This fix ensures the correct d.ts generation. The transpile procees now creates an index.d.ts and all TS UI5 modules will declare its module with the fully qualified name:
In addition, the generated control interfaces, the
.gen.d.ts
files need to also be converted to declare the module with the fully qualified name and put the imports into the module declaration.The
ui5-tooling-transpile
tooling extension to properly generate the type definitions including theindex.d.ts
which ensures that e.g. TypeScript UI5 applications consuming UI5 libraries will get code completion by just adding thetypeRoots
configuration:Snippet taken from: https://github.com/ui5-community/ui5-ecosystem-showcase/blob/223b9f54745c397f6bf7aec430d45d5a9a33c354/showcases/ui5-tsapp/tsconfig.json:
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 thetypeRoots
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 tonode_modules/@types
so that the types from definitely typed can be found.