terraform-cdk icon indicating copy to clipboard operation
terraform-cdk copied to clipboard

workspace: missing dependencies

Open felixmagnus opened this issue 2 years ago • 3 comments

Expected Behavior

All dependencies used by a package should be declared in that packages package.json file. This ensures a safe package resolution & install process independent from the used package manager.

Actual Behavior

Certain dependencies in different packages are missing an explicit declaration in their respective package.json.

Complete List of missing dependencies by package:

packages/@cdktf/cli-core

  • ci-info is missing (usage is in ./src/lib/error-reporting.ts)

packages/@cdktf/commons

  • strip-ansi is missing (usage is in ./src/util.ts)

packages/@cdktf/hcl2cdk

  • fs-extra is missing (usage is in ./test/helpers/convert.ts --> so should be a devDependency probably?)
  • execa is missing (usage is in ./test/helpers/convert.ts --> so should be a devDependency probably?)
  • codemaker is missing (usage is in ./lib/coerceType.ts)

packages/@cdktf/provider-generator

  • glob is missing (usage is in ./lib/get/constructs-maker.ts)

packages/@cdktf/provider-schema

  • deepmerge is missing (usage is in ./src/read.ts)

I stumbled across this issue due to our pipeline sometimes failing during a cdktf deploy with error messages like:

Cannot find module 'strip-ansi'
...
Cannot find module 'glob'
...
etc.

As we use turborepo + pnpm (which I know is not officially supported by cdktf) and you have not stumbled over those missing dependency declarations so far my guess is that yarn / lerna workspaces are in some way more forgiving wrt. these declarations (compared to pnpm).

Besides fixing the issue for our (to be fair unsupported pnpm) usecase I nevertheless think it would be sensible to declare the dependencies explicitly as mentioned above.

Steps to Reproduce

  • Clone the repository.
  • Go into any of the mentioned packages:
    • packages/@cdktf/cli-core
    • packages/@cdktf/commons
    • packages/@cdktf/hcl2cdk
    • packages/@cdktf/provider-generator
    • packages/@cdktf/provider-schema
  • Run npx depcheck (See depcheck) and look at the "Missing dependencies" section

Versions

I cloned the repository on commit: e207a50aae198ac7cc78dc40356b35946c9d1ad8 (origin/main at that time) Tags: v0.19.0-pre.34, v0.19.0

Providers

No response

Gist

No response

Possible Solutions

Add the missing dependencies as explicit declarations in the respective package.json files.

Workarounds

Anything Else?

Please let me know if you need anything else from me, I'll be glad to support further to get the issue resolved.

References

No response

Help Wanted

  • [X] I'm interested in contributing a fix myself

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

felixmagnus avatar Oct 19 '23 12:10 felixmagnus

Thank you for pointing this out, I opened a PR to fix this and added one that runs depcheck on PRs so we don't run into this issue again :) yarn is very convenient, but it makes it easy to forget adding the dependencies when everything runs as expected

DanielMSchmidt avatar Oct 20 '23 13:10 DanielMSchmidt

No worries! Great to hear, thanks for reacting so quickly 🙂

felixmagnus avatar Oct 20 '23 13:10 felixmagnus

yarn is very convenient, but it makes it easy to forget adding the dependencies when everything runs as expected

Just enable Yarn PnP and you will see all possible dependency issues, including require / dynamic imports )

BTW, there is additional missed dependency @cdktf/node-pty-prebuilt-multiarch in cdktf-cli. It's not a direct dependency, so you can't find any import / require in code. But, it present in the compiled bundle/bin/cmds/handlers.js: image

The root cause is: https://github.com/hashicorp/terraform-cdk/blob/e207a50aae198ac7cc78dc40356b35946c9d1ad8/packages/cdktf-cli/build.ts#L72-L82 package.json doesn't contains that dependency.

koshic avatar Oct 28 '23 12:10 koshic