cdk-monitoring-constructs icon indicating copy to clipboard operation
cdk-monitoring-constructs copied to clipboard

Can only be used with specific aws-cdk-lib versions due to alpha module peer dependencies

Open hteek opened this issue 2 years ago • 13 comments

Version

v1.18.4

Steps and/or minimal code example to reproduce

The peer-dependencies are specified in a way that is not working when using any cdk version higher than 2.18.0.

    "@aws-cdk/aws-apigatewayv2-alpha": "^2.18.0-alpha.0",
    "@aws-cdk/aws-appsync-alpha": "^2.18.0-alpha.0",
    "@aws-cdk/aws-redshift-alpha": "^2.18.0-alpha.0",
    "@aws-cdk/aws-synthetics-alpha": "^2.18.0-alpha.0",

These only match 2.18.0-alpha.0, 2.18.0-alpha.1, 2.18.0-alpha.2 and so on but not 2.19.0-alpha.0 or in my case 2.22.0-alpha.0

Expected behavior

Any minor version higher than 2.18.0-alpha.0 can be used.

Actual behavior

Error: Error: Declared dependency on version ^2.18.0-alpha.0 of @aws-cdk/aws-apigatewayv2-alpha, but version 2.22.0-alpha.0 was found

Other details

I couldn't find a way to make it work with overrides or resolutions and to make it even worse there is no notation that matches 2.18.0-alpha.0 and 2.19.0-alpha.0 other than 2.18.0-alpha.0 || 2.19.0-alpha.0.

hteek avatar Aug 10 '22 20:08 hteek

Assuming you're using npm >= 7, you should be able to configure legacy-peer-deps (docs) to allow for newer versions to be installed, at the risk that the alpha modules' APIs may not match up.

One solution would be to occasionally bump our peer dependencies' versions, but that may be annoying for users.

echeung-amzn avatar Aug 10 '22 20:08 echeung-amzn

There is no problem when installing the new versions. The problem is that the jsii compiler is not able to compile my construct with the newer versions. Sorry if that was not clear.

$ jsii
Error: Error: Declared dependency on version ^2.18.0-alpha.0 of @aws-cdk/aws-apigatewayv2-alpha, but version 2.22.0-alpha.0 was found
    at DependencyResolver.discoverDependencyTree (/*REMOVED*/node_modules/jsii/lib/project-info.js:131:23)
    at DependencyResolver.loadAssemblyAndRecurse (/*REMOVED*/node_modules/jsii/lib/project-info.js:180:20)
    at DependencyResolver.resolveDependency (/*REMOVED*/node_modules/jsii/lib/project-info.js:168:34)
    at DependencyResolver.discoverDependencyTree (/*REMOVED*/node_modules/jsii/lib/project-info.js:128:35)
    at Object.loadProjectInfo (/*REMOVED*/node_modules/jsii/lib/project-info.js:51:31)
    at /*REMOVED*/node_modules/jsii/bin/jsii.js:78:81
    at Object.<anonymous> (/*REMOVED*/node_modules/jsii/bin/jsii.js:104:3)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)

hteek avatar Aug 11 '22 19:08 hteek

Is this in the context of building a construct that uses cdk-monitoring-constructs, or a CDK project that isn't using TypeScript?

echeung-amzn avatar Aug 12 '22 17:08 echeung-amzn

I am trying to build a construct in TypeScript that is using ˋcdk-monitoring-constructsˋ

hteek avatar Aug 14 '22 19:08 hteek

A few options that I can see:

  1. Occasionally bump our peer dependencies, which would be considered a breaking change and therefore a major version bump. We'll notably need to do this anyway if the alpha modules have breaking changes that affect our usages or when they go stable.
  2. Modify the alpha module peer dependencies to include all possible versions (e.g. 2.18.0-alpha.0 || 2.19.0-alpha.0), occasionally updating it to keep up. Our projen setup doesn't really lend itself to doing matrix-based tests against all possible versions that easily.
  3. Try to drop the dependencies. This could entail splitting out any alpha functionality into a separate package (but that would have the same issues as we do today), or duplicating the needed features in our codebase (interfaces and metric declarations; we could probably keep the alpha modules for unit testing purposes).

@voho @ayush987goyal Any thoughts?

echeung-amzn avatar Aug 16 '22 18:08 echeung-amzn

I am mostly inclined towards approach 2 which does not change any exiting behavior by a lot and also addresses this use-case. On a side note, have we checked if > operators etc. work instead of ^ ?

ayush987goyal avatar Aug 17 '22 04:08 ayush987goyal

On a side note, have we checked if > operators etc. work instead of ^ ?

Yeah, it doesn't work. You can try it out on https://semver.npmjs.com/

echeung-amzn avatar Aug 17 '22 14:08 echeung-amzn

A quick revive of this thread Do we have a work around for this ? Or is the only solution a npm i --force in the meantime for consumer of the library who are using aws-cdk > 2.18 ?

svrielynck avatar Nov 01 '22 19:11 svrielynck

Workarounds would be to use npm i --force, use legacy-peer-deps, or use overrides.


I'm personally inclined to go with option 1 (occasionally bump our peer dependencies as a major version bump), since it'd also allow us to use newer AWS CDK CloudWatch features like new widget types. The eventual updates to handle alpha modules graduating to stable would necessitate a similar change anyhow.

@ayush987goyal Any concerns with the above?

echeung-amzn avatar Nov 01 '22 20:11 echeung-amzn

Checked the issues so far but it seems I'm forced to include all of the alpha dependencies which are currently sitting inside of the cdk-monitoring package as well even though I'm not using them

node_modules/cdk-monitoring-constructs/lib/monitoring/aws-synthetics/SyntheticsCanaryMetricFactory.d.ts(1,24): error TS2307: Cannot find module '@aws-cdk/aws-synthetics-alpha' or its corresponding type declarations.
52

After adding this particular dependency I'm having run-time failures because I don't have the exact matching versions

● Test suite failed to run
--
61 |  
62 | Cannot find module '@aws-cdk/aws-redshift-alpha' from 'node_modules/cdk-monitoring-constructs/lib/facade/MonitoringAspect.js'
63 |  
64 | Require stack:
65 | node_modules/cdk-monitoring-constructs/lib/facade/MonitoringAspect.js
66 | node_modules/cdk-monitoring-constructs/lib/facade/MonitoringFacade.js
67 | node_modules/cdk-monitoring-constructs/lib/facade/index.js
68 | node_modules/cdk-monitoring-constructs/lib/index.js

is it an expected behavior ?

svrielynck avatar Nov 03 '22 19:11 svrielynck

I'm having the same issue as @svrielynck described, adding all of the alpha as deps fixed that, but is not the most elegant solution and is also not described as requirement in the README

PatrykMilewski avatar Mar 05 '23 18:03 PatrykMilewski

The current strategy is just to wait for the existing alpha dependencies to go stable (2 of 4 have gone stable thus far), and no longer adding any dependencies on alpha modules as part of this library.

The easiest way of dealing with the conflicts right now is using overrides to explicitly use whatever versions your project directly declares a dependency on, for example:

{
  "dependencies": {
    "cdk-monitoring-constructs": "^6.0.0",

    // These are notably a higher version that what the used version
    // of cdk-monitoring-constructs depends on
    "@aws-cdk/aws-apigatewayv2-alpha": "^2.106.0-alpha.0",
    "@aws-cdk/aws-redshift-alpha": "^2.106.0-alpha.0",
    "aws-cdk-lib": "^2.106.0",

    // etc.
  },
  "overrides": {
    "cdk-monitoring-constructs": {
      "@aws-cdk/aws-apigatewayv2-alpha": "$@aws-cdk/aws-apigatewayv2-alpha",
      "@aws-cdk/aws-redshift-alpha": "$@aws-cdk/aws-redshift-alpha"
    }
  }
}

echeung-amzn avatar Nov 16 '23 14:11 echeung-amzn

We're currently left with only @aws-cdk/aws-redshift-alpha after #464.

echeung-amzn avatar Dec 01 '23 17:12 echeung-amzn