[BUG] @cap-js/cds-types v0.6.0 - Post install script not working

geert-janklaps commented 1 month ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current Behavior

Currently when enabling @sap/cds with typescript (using cds --add typescript, typer) and after reinstalling the dependencies, the post installation script doesn't create the symbolic link to the @types folder.

This results in not being able to use the following statement in ts files: import cds from '@sap/cds'.

This can be fixed by manually creating the symbolic link as follows (on Linux): ln -s /workspaces/advanced-output-management/node_modules/@cap-js/cds-types node_modules/@types/sap__cds

I'm running the project in a dev container with image: mcr.microsoft.com/devcontainers/typescript-node:1-20-bookworm

Expected Behavior

The post install script to automatically create the necessary symbolic link

References

not applicable

Versions

advanced-output-manage
@cap-js/asyncapi	1.0.1
@cap-js/cds-typer	0.23.0
@cap-js/cds-types	0.6.0
@cap-js/hana	1.1.0
@cap-js/openapi	1.0.4
@cap-js/sqlite	1.7.3
@sap/cds	8.0.3
@sap/cds-compiler	5.0.6
@sap/cds-dk	8.0.2
@sap/cds-dk (global)	8.0.2
@sap/cds-fiori	1.2.7
@sap/cds-foss	5.0.1
@sap/cds-mtxs	2.0.2
@sap/eslint-plugin-cds	3.0.4
Node.js	v20.14.0
home	/workspaces/advanced-output-management/node_modules/@sap/cds

Anything else? Logs?

Devcontainer.json configuration:

// For format details, see https://aka.ms/devcontainer.json. For config options, see the
// README at: https://github.com/devcontainers/templates/tree/main/src/typescript-node
{
    "name": "advanced-output-management-devcontainer",
    // Or use a Dockerfile or Docker Compose file. More info: https://containers.dev/guide/dockerfile
    "image": "mcr.microsoft.com/devcontainers/typescript-node:1-20-bookworm",
    // Features to add to the dev container. More info: https://containers.dev/features.
    // "features": {},
    // Use 'forwardPorts' to make a list of ports inside the container available locally.
    "forwardPorts": [
        4004,
        5000
    ],

    // Uncomment to connect as root instead. More info: https://aka.ms/dev-containers-non-root.
    "remoteUser": "root"
}

dinurp commented 1 month ago

This issue is causing errors in initializing a sample cap project.

PS D:\temp> cds init cap-app-sample
Creating new CAP project in .\cap-app-sample

Adding feature 'nodejs'...

Successfully created project. Continue with 'cd cap-app-sample'.
Find samples on https://github.com/SAP-samples/cloud-cap-samples
Learn about next steps at https://cap.cloud.sap
PS D:\temp> cd .\cap-app-sample\
PS D:\temp\cap-app-sample> cds add sample
Adding feature 'sample'...

Successfully added features to your project.
PS D:\temp\cap-app-sample> cds -v -i

| cap-app-sample         | <Add your repository here>                                                        |
| ---------------------- | --------------------------------------------------------------------------------- |
| @cap-js/asyncapi       | 1.0.1                                                                             |
| @cap-js/openapi        | 1.0.4                                                                             |
| @sap/cds               | 8.0.3                                                                             |
| @sap/cds-compiler      | 5.0.6                                                                             |
| @sap/cds-dk (global)   | 8.0.2                                                                             |
| @sap/cds-fiori         | 1.2.7                                                                             |
| @sap/cds-foss          | 5.0.1                                                                             |
| @sap/cds-mtxs          | 2.0.2                                                                             |
| @sap/eslint-plugin-cds | 3.0.4                                                                             |
| Node.js                | v18.16.0                                                                          |
| home                   | C:\Users\me\AppData\Roaming\npm\node_modules\@sap\cds-dk\node_modules\@sap\cds |

PS D:\temp\cap-app-sample> npm i
npm ERR! code 1
npm ERR! path D:\temp\cap-app-sample\node_modules\@cap-js\cds-types
npm ERR! command failed
npm ERR! command C:\Windows\system32\cmd.exe /d /s /c ./scripts/postinstall.js
npm ERR! '.' is not recognized as an internal or external command,
npm ERR! operable program or batch file.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\me\AppData\Local\npm-cache\_logs\2024-07-17T07_23_13_223Z-debug-0.log

package.json has the following entry

  "devDependencies": {
    "@cap-js/sqlite": "^1",
    "@cap-js/cds-types": "^0.6"
  },

changing this to the following allows to proceed with initializing sample project.

  "devDependencies": {
    "@cap-js/sqlite": "^1",
    "@cap-js/cds-types": "^0.2"
  },

daogrady commented 1 month ago

Hi everyone,

thanks for pointing out this problem on Windows. We will see what we can do to unblock Windows users.

Best, Daniel

geert-janklaps commented 1 month ago

Hi @daogrady,

I'm not sure if you are aware, but this issue can also be solved without creating a symbolic link. I've adapted my tsconfig.json with typeRoots like so:

{
  "compilerOptions": {
    "target": "ESNext",
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true,
    "skipLibCheck": true,
    "sourceMap": true,
    "allowJs": true,
    "paths": {
      "#cds-models/*": [
        "./@cds-models/*/index.ts"
      ]
    },
    "typeRoots": [
      "./node_modules/@types",
      "./node_modules/@cap-js"
    ]
  }
}

This is working perfectly fine without the need of a symbolic link. (similar setup like the UI5 team does for the UI5 types) With that minor difference, that in this case we can't specify the @cap-js/cds-types package explicitly (probably because of the way the package structure is setup, in this case the types are in a dist folder while in the UI5 types these are in a types folder)

Anyway, this feels like a more solid and standard TypeScript configuration. Might be worth considering for you guys :) (And in the meanwhile, takes away the complexity of handling different OS's)

Cheers, Geert-Jan

daogrady commented 1 month ago

Hi Geert-Jan,

thanks for pointing this out! We have considered an additional typeRoots entry (among a few other options), but wanted to not burden the user with any additional config they would have to add to their project (for JS projects, this could even mean the user would need an entire jsconfig.json for just this entry).

If having the typeRoots entry works for you then that is perfectly fine for your project to use! 👍 Still, I hope to be able to come up with a solution that works for everyone out of the box.

Best, Daniel

daogrady commented 1 month ago

Hi everyone,

we just released cds-types 0.6.1 which contains a fix for the post install script on windows which should address this issue. Should the problem persist, feel free to reopen this issue or open a follow-up.

Best, Daniel

geert-janklaps commented 1 month ago

Hi @daogrady,

I opened the issue on Linux, not on Windows. I've just checked, v0.6.1 still causes issues on Linux. I get the same issue on my local installation and in a container running as a GitHub action:

Cheers, Geert-Jan

daogrady commented 1 month ago

Hi Geert-Jan,

sorry for the inconvenience! Is this the same error you encountered originally? It looks like this error pops up in the context of building/ bundling?

Best, Daniel

geert-janklaps commented 1 month ago

Hi @daogrady,

Seems like a slightly different situation indeed. What seems to be the issue I'm encountering:

Existing project running @sap/cds @ 7.9.3
No explicit installation of @cap-js/types
Running npm install -> works fine (adds dependency to @cap-js/cds-types @ 0.2.0)
Running npm update --package-lock-only -> causes error (adds dependency to @cap-js/cds-types @ 0.6.1)

Based on the configuration the last situation shouldn't happen (as @cap-js/cds-types has peer dependency to @sap/cds @ 8)

I'm able to fix this by explicitly installing @cap-js/cds-types @0.2.0 in the project.

I also retested the initial issue (on cds v8) and this issue does seem to be resolved. So maybe lets close this issue, I'll do some further analysis on the existing project and if needed I'll open a new issue.

Cheers, Geert-Jan

daogrady commented 1 month ago

Hi Geert-Jan,

thanks for the analysis of the problem! That is indeed an issue, as cds-types 0.6 should only be used in combination with cds 8. Pipelines pulling in cds-types 0.6 despite its peerDependency to cds 8 is not the desired behaviour and will lead to problems. I will investigate this.

Best, Daniel

daogrady commented 1 month ago

Hi @daogrady,

Seems like a slightly different situation indeed. What seems to be the issue I'm encountering:

Existing project running @sap/cds @ 7.9.3

No explicit installation of @cap-js/types

Running npm install -> works fine (adds dependency to @cap-js/cds-types @ 0.2.0)

Running npm update --package-lock-only -> causes error (adds dependency to @cap-js/cds-types @ 0.6.1)

Based on the configuration the last situation shouldn't happen (as @cap-js/cds-types has peer dependency to @sap/cds @ 8)

I'm able to fix this by explicitly installing @cap-js/cds-types @0.2.0 in the project.

I also retested the initial issue (on cds v8) and this issue does seem to be resolved. So maybe lets close this issue, I'll do some further analysis on the existing project and if needed I'll open a new issue.

Cheers, Geert-Jan

@chgeo FYI

daogrady commented 1 month ago

@geert-janklaps @hakimio

thanks for pointing out this incompatibility with cds7 projects. The problem stems from an open dependency range in cds7 which we will fix asap.

chgeo commented 1 month ago

@daogrady that's seems to be the case I was worried about yesterday in out discussion. Yes, it's probably worth closing the dependency range in the next sap/cds patch 7.9.x

dinurp commented 1 month ago

Thanks for the fix. Initializing a sample project with CDS@8 works fine now with this this fix.

PS D:\temp\cap-app-sample> npm ls
cap-app-sample@1.0.0 D:\temp\cap-app-sample
+-- @cap-js/cds-types@0.6.1
+-- @cap-js/sqlite@1.7.3
+-- @sap/cds@8.0.3
`-- express@4.19.2

devinea commented 1 month ago

@daogrady, @chgeo I think part of the problem with CDS V7 projects is that the postinstall script assumes the folder location for the symlink target. https://github.com/cap-js/cds-types/blob/main/scripts/postinstall.js#L17 Would be be more robust to use const pathToModule = require.resolve('@cap-js/cds-types'); so the symlink always has a valid target

chgeo commented 1 month ago

Could point. Should check if this is more robust.

daogrady commented 1 month ago

Hi @devinea,

thanks for the suggestion! While I see the general appeal of the more generic approach, I have a hard time seeing the case in this specific scenario. As the script is executed as part of the installation of @cap-js/cds-types, it should always find that package. Could you please point out the circumstances under which targeting the specific folder location is incorrect?

Best, Daniel

daogrady commented 3 weeks ago

Hi everyone,

has been rather quiet around the install script lately, so I assume this has been fixed for now. Therefore closing the issue. 🙂

Best, Daniel

cap-js / cds-types