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

Maybe rethink cdkDependencies in awscdk-construct #577

Closed
Miradorn opened this issue Feb 26, 2021 · 1 comment · Fixed by #581
Closed

Maybe rethink cdkDependencies in awscdk-construct #577

Miradorn opened this issue Feb 26, 2021 · 1 comment · Fixed by #581

Comments

@Miradorn
Copy link
Contributor

Hi there and thanks for the great project!

We're currently building https://github.com/superluminar-io/super-eks/ with it and are very happy.

Unfortunately we're either doing smth wrong or there might be a conceptual problem in the way cdkDependencies are handled.
The Problem (and solution i think) is described in this blogpost https://dev.to/udondan/correctly-defining-dependencies-in-l3-cdk-constructs-45p , basically: defining cdk deps as real dependencies puts limitations on your downward consumers, therefore they should be added to peerDependencies and devDependencies, as this enables to use them during development but allows consumers to use their own specified version.

If this is correct thinking on my side, this line https://github.com/projen/projen/blob/main/src/awscdk-construct.ts#L150 would probably has to be changed? Or is there another way that I sould be using? Also the comment (and therefore reasoning) above the method is probably outdated, since npm 7 is actually installing peerDependencies (https://github.blog/2021-02-02-npm-7-is-now-generally-available/#peer-dependencies)

If you're agreeing with me I can submit a PR if you'd like.
Thanks again!

@eladb
Copy link
Contributor

eladb commented Feb 28, 2021

At the moment we add deps as both "peer" and normal deps since previous versions of npm did not install peer dependencies automatically. This means that users would need to manually add all the transitive deps to their projects.

This will be solved once we released CDK 2.0 which will be published as a single module, so libs could just define it as a peer dep and consumers will bring the correct version.

Happy to accept a contribution to add a switch which disables this behavior though, especially given its a hack.

@mergify mergify bot closed this as completed in #581 Mar 1, 2021
mergify bot pushed a commit that referenced this issue Mar 1, 2021
Allows disabling the default behavior in which CDK dependencies are also added as normal dependencies (and not just peer dependencies).

Resolves #577

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
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

Successfully merging a pull request may close this issue.

2 participants