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

Looser peerDependencies for CDK modules #23

Merged
merged 2 commits into from
May 6, 2021
Merged

Looser peerDependencies for CDK modules #23

merged 2 commits into from
May 6, 2021

Conversation

plumdog
Copy link
Contributor

@plumdog plumdog commented May 4, 2021

While I don't believe this is totally accurate, as such, I suspect that there are CDK versions below 1.64 where this module will work fine, but it looks like npm is now stricter when peer dependencies are not satisfied.

Attempting to install this module where I'm using an older version of CDK, I get an error like:

$ npm i --save cdk-datadog-integration
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! While resolving: myproject@1.0.0
npm ERR! Found: @aws-cdk/aws-cloudwatch@1.102.0
npm ERR! node_modules/@aws-cdk/aws-cloudwatch
npm ERR!   peer @aws-cdk/aws-cloudwatch@"1.102.0" from @aws-cdk/aws-lambda@1.102.0
npm ERR!   node_modules/@aws-cdk/aws-lambda
npm ERR!     peer @aws-cdk/aws-lambda@"1.102.0" from @aws-cdk/aws-cloudformation@1.102.0
npm ERR!     node_modules/@aws-cdk/aws-cloudformation
npm ERR!       peer @aws-cdk/aws-cloudformation@"^1.64.1" from cdk-datadog-integration@1.1.1
npm ERR!       node_modules/cdk-datadog-integration
npm ERR!         cdk-datadog-integration@"*" from the root project
npm ERR!   peer @aws-cdk/aws-cloudwatch@"1.102.0" from @aws-cdk/aws-applicationautoscaling@1.102.0
npm ERR!   node_modules/@aws-cdk/aws-applicationautoscaling
npm ERR!     peer @aws-cdk/aws-applicationautoscaling@"1.102.0" from @aws-cdk/aws-lambda@1.102.0
npm ERR!     node_modules/@aws-cdk/aws-lambda
npm ERR!       peer @aws-cdk/aws-lambda@"1.102.0" from @aws-cdk/aws-cloudformation@1.102.0
npm ERR!       node_modules/@aws-cdk/aws-cloudformation
npm ERR!         peer @aws-cdk/aws-cloudformation@"^1.64.1" from cdk-datadog-integration@1.1.1
npm ERR!         node_modules/cdk-datadog-integration
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer @aws-cdk/aws-cloudwatch@"1.55.0" from @aws-cdk/aws-ec2@1.55.0
npm ERR! node_modules/@aws-cdk/aws-ec2
npm ERR!   @aws-cdk/aws-ec2@"1.55.0" from the root project

That is, npm is - not unreasonably - complaining because my package.json has "@aws-cdk/aws-ec2": "1.55.0" but this module has "@aws-cdk/aws-cloudformation": "^1.64.1", and each of those wants a compatible @aws-cdk/aws-cloudwatch, which are incompatible. It looks like this is different behaviour between npm 6 and 7, where 6 was more lenient, but 7 errors.

One option would be for me to upgrade CDK. In my case that is not possible.
Another would be to update this modules package.json to have the loosest functioning peer dependencies, but alas, I'm just not that diligent.
Thus, I have arrived at the changes that I have here.

@blimmer
Copy link
Owner

blimmer commented May 5, 2021

Hey @plumdog - thanks for this PR. I'm sure there's some version earlier than 1.64.1 that breaks this library but I'm OK with loosening this requirement as long as there's a note about compatibility.

Would you be willing to do two things with this PR?

  1. Add a comment in README.md that this library is tested with 1.xx (1.55.0 if you've tested against it, 1.64.1 if not). A disclaimer should be included that "your mileage may vary" with versions earlier than the tested version.
  2. Add <2 to the peer dependencies while we're updating it. v2 is a breaking change and I'm assuming I'll need to do some updates to be compatible when it's released.

Thanks for your contribution!

@plumdog
Copy link
Contributor Author

plumdog commented May 6, 2021

It does indeed work fine with 1.55.0, based on my testing - which is to say: it successfully creates the stacks I expect, and metrics and logs are arriving in Datadog, so I consider that "passed testing". I've added a note to the README to say as much.

I believe that the version constraints already in package.json do exclude a new major version. NPM's docs have the example:

^1.2.3 := >=1.2.3 <2.0.0

so I think the version constraint is already set to the right thing. See https://docs.npmjs.com/cli/v7/using-npm/semver#caret-ranges-123-025-004 for excessive detail.

@blimmer blimmer merged commit 0bbde49 into blimmer:master May 6, 2021
@blimmer
Copy link
Owner

blimmer commented May 6, 2021

Sadly, it appears that the build is broken now, so the release did not succeed. I don't have time to take a look at it for a few days, so I opened #29

@blimmer blimmer mentioned this pull request May 8, 2021
@blimmer
Copy link
Owner

blimmer commented May 8, 2021

@plumdog - sorry for the delay in taking a look at this. 1.21.0 should work for you now. Thanks again for your contribution!

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 this pull request may close these issues.

None yet

2 participants