SAP / ui5-typescript

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

Folder structure of @sapui5/types is wrong #461

Closed rasmk closed 2 months ago

rasmk commented 2 months ago

When @sapui5/types is referred to from typeRoots in tsconfig.json, running tsc command (my tsconfig.json is below) produces an error:

error TS2688: Cannot find type definition file for 'node_modules'.
  The file is in the program because:
    Entry point for implicit type library 'node_modules'

According to this TS issue: https://github.com/microsoft/TypeScript/issues/27956#issuecomment-430849185, each subfolder of the folder that is referred to from typeRoots should contain index.d.ts file (directly beneath).

@sapui5/types has the following type structure:

Clearly, node_modules does not follow this requirement. This causes the trouble described above. If I manually pull jquery subfolder two levels up and remove node_modules the tsc runs without any problem.

My tsconfig.json file is like below. It is a fiori app being a part of CAP project with npm workspaces enabled. Therefore it refers to node_modules of the main project folder.

{
  "compilerOptions": {
    "target": "es2022",
    "module": "es2022",
    "skipLibCheck": true,
    "allowJs": true,
    "strict": true,
    "strictPropertyInitialization": false,
    "moduleResolution": "node",
    "rootDir": "./webapp",
    "outDir": "./dist",
    "baseUrl": "./",
    "paths": {
      "myapp/*": ["./webapp/*"]
    },
    "typeRoots": ["../../node_modules/@types", "../../node_modules/@sapui5/types/test"]
  },
  "include": ["./webapp/**/*"]
}

Expected behavior The working folder structure should be flat, like:

Actually, it would be a good idea to rename types subfolder to sapui5 then, to make it clear, what it is about. But this is a matter of taste, also not critical.

lemaiwo commented 2 months ago

I think this is similar to this one which is known and documented: https://github.com/ui5-community/generator-ui5-ts-app-fcl/issues/5

rasmk commented 2 months ago

Thanks for the hint @lemaiwo. It is similar indeed. Still, I expect the root cause solved and contents of @sapui5/types to be fixed rather than having me applying a manual workaround.

akudev commented 2 months ago

@rasmk I think two things have to be clarified:

First, if you check the actual content of @sapui5/types on https://www.npmjs.com/package/@sapui5/types?activeTab=code, you will see that there is NO node_modules folder:

image

That folder is only created on your own system during installation of project dependencies (npm install) in cases where your application has a duplicate dependency to packages which are also required by @sapui5/types (in your case @types/jquery, it could also be @types/qunit) and if these dependencies have a different version. So basically the root cause of having the node_modules folder in @sapui5/types is the dependency in your application.

Second, the presence of this folder is only a problem when your application project sets typeRoots in its tsconfig. But in the issue which @lemaiwo linked above, it is mentioned that it's actually recommended not to use typeRoots (for exactly this reason), but types - see this comment. Also the ui5-typescript-tutorial and our sample apps like this one and the Easy-UI5 app templates use types. We are aware that this unfortunately requires ALL needed type packages to be explicitly listed because TypeScript works like this.

So your choices are to either

The whole topic is a bit complex, I know, but essentially both, the node_modules folder (via your dependency) and the typeRoots setting which makes the folder a problem, are not coming from @sapui5/types, but from your application project setup, so we cannot do anything about it. That's the pain of having types in an npm package which is not in the @types namespace. The only thing we could do would be removing our own dependency to the jQuery and qUnit types, but this would mean that every user of our types would get TypeScript errors by default because of the missing types. Everyone would have to add them manually and make sure to use the best-fitting version and update them when UI5 updates the library. We had it like that in the beginning, but it was a mess and hard to explain. It was also logically wrong, because it is UI5, not the application, which loads jQuery and QUnit and decides which version of it to load. Becoming part of the @types namespace, on the other hand, does not seem feasible for legal reasons, as the non-open-source APIs would need to be published as open source in the DefinitelyTyped repo which owns that namespace. For OpenUI5 we do this, though. (And to be honest, it's a pain, but that's a different topic.)

It's all also described in https://sap.github.io/ui5-typescript/known-issues.html

rasmk commented 2 months ago

Thanks for the detailed explanation @akudev. I see the challenges you are facing.

I have a CAP/MTA project with npm workspaces enabled. Which brings the npm dependencies to node_modules folder of the root project. I do not have jquery referenced anywhere as a dependency. The reason the node_modules was added to the @sapui5/types folder was, one of the fiori apps was referring to a different version of @sapui5/types module than the others. Bringing them all to the same version seems to solve the problem. As we have over 30 apps, it may be difficult to keep them harmonized all the time though.

The mentioned workaround with types does not seem to work in the workspace project. With "types": ["@sapui5/types"] and typeRoots removed, the UI5 types are not found in the project.

For now my solution is to keep the references harmonized, so that node_modules folder is not created. If this fails to work long term, I will copy the @sapui5/types into my project and maintain it manually.

While I have my workarounds, is node_modules subfolder needed in any circumstances inside @sapui5/types? Or is it just a side-effect? If the latter, perhaps it could be removed with a postinstall npm script? No idea, if this will help, to be honest. But perhaps worth trying.

akudev commented 2 months ago

Yeah, I get it that concrete usage scenarios are also often more complex than one would think, just like it is the case on our provider side.

is node_modules subfolder needed in any circumstances inside @sapui5/types? Or is it just a side-effect?

The node_modules folder is, as I wrote, nothing we create or we want or we require. It is created in this location as soon as the same package is required in different versions. By default all dependencies go to the root-level node_modules, but when there is already a @types/jquery with a different version, where should npm put the other one? To solve this conflict, it simply creates another node_modules folder below the package that also required @types/jquery - inside our @sapui5/types. I'm sure other package managers like yarn or pnpm lead to a slightly different behavior. So I'd call it a side-effect of npm's strategy to avoid name clashes when saving dependencies. If npm would e.g. append the version of the package to the folder name instead of using a different place, then we wouldn't have this issue.

one of the fiori apps was referring to a different version of @sapui5/types module than the others

I see. Yes, as result the same applies: two different jQuery types versions are required.

With "types": ["@sapui5/types"] and typeRoots removed, the UI5 types are not found in the project.

Umm... maybe try a relative path when the package is not in the default location ./node_modules. Probably starts with ../../in your case to ascend to the root node modules.

With your approach, there is one thing I would like to mention: once the unwanted node_modules folder is there, it's not so easy to get rid of it. Just bringing the version numbers in sync again does not remove it. You really have to delete it and delete package-lock.json, then it should work.

rasmk commented 2 months ago

Thanks for the hint @akudev My solution turned out to be as simple as "postinstall": "rimraf node_modules/@sapui5/types/node_modules" in my package.json.

You can keep the issue open if you like to follow it up or close it. I am good with what I have.

akudev commented 2 months ago

Alright. I'm closing it, as there isn't anything we can do to prevent the creation of the folder by npm under the mentioned circumstances and the options are documented.