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

Folder structure of @sapui5/types is wrong #461

Closed
rasmk opened this issue Jun 22, 2024 · 7 comments
Closed

Folder structure of @sapui5/types is wrong #461

rasmk opened this issue Jun 22, 2024 · 7 comments

Comments

@rasmk
Copy link

rasmk commented Jun 22, 2024

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: microsoft/TypeScript#27956 (comment), 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:

  • types
    • index.d.ts
    • ...
  • node_modules
    • @types
      • jquery
        • index.d.ts
        • ...

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:

  • types
    • index.d.ts
    • ...
    • jquery
      • index.d.ts
      • ...

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
Copy link

lemaiwo commented Jun 22, 2024

I think this is similar to this one which is known and documented: ui5-community/generator-ui5-ts-app-fcl#5

@rasmk
Copy link
Author

rasmk commented Jun 22, 2024

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

akudev commented Jun 24, 2024

@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

  • use types instead of typeRoots for referencing the type packages or
  • to remove your dependencies to jQuery/QUnit type packages which are also required by @sapui5/types or
  • require the same version that @sapui5/types requires.
    Actually the latter usually also does make a lot of sense, because UI5 comes with a specific version of jQuery and QUnit included and @sapui5/types has a dependency to a well-matching version of their types. If you reference a different version of the jQuery/QUnit types, then the version you require will be a less good match with the actual library.

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
Copy link
Author

rasmk commented Jun 24, 2024

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

akudev commented Jun 24, 2024

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
Copy link
Author

rasmk commented Jun 26, 2024

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

akudev commented Jun 26, 2024

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.

@akudev akudev closed this as completed Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants