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

Update semver usage in cdk packages #3711

Closed
KnisterPeter opened this issue Aug 19, 2019 · 15 comments
Closed

Update semver usage in cdk packages #3711

KnisterPeter opened this issue Aug 19, 2019 · 15 comments
Assignees
Labels
bug This issue is a bug. p0

Comments

@KnisterPeter
Copy link
Contributor

❓ General Issue

Semver defined in internal packages.

The Question

All CDK packages use semver for internal dependencies. That means e.g. @aws-cdk/aws-cloudfront defines @aws-cdk/aws-iam as dependecies with the version of itself but with a caret.

E.g. version 1.2.0 of aws-cloudfront dependes on ^1.2.0 of aws-iam which is wrong since it requires exaclty 1.2.0.

This could easily leads to bugs which are hard to debug/understand.

@KnisterPeter KnisterPeter added the needs-triage This issue or PR still needs to be triaged. label Aug 19, 2019
@skinny85
Copy link
Contributor

skinny85 commented Aug 19, 2019

I confirm this is a problem:

$ cat package.json 
{
	"dependencies": {
		"@aws-cdk/core": "1.3.0"
	}
}

$ cat node_modules/\@aws-cdk/core/package.json | grep version
  "version": "1.3.0"

$ cat node_modules/\@aws-cdk/cx-api/package.json | grep version
  "version": "1.4.0"

There is no way to downgrade to 1.3.0 after 1.4.0 has been released.

@carlosrfernandez
Copy link
Contributor

carlosrfernandez commented Aug 20, 2019

@statik Well technically there is... But it is a pain, I had to do it...

You have first uninstall -g aws-cdk, then npm i -g aws-cdk@1.3.0.

Then edit the packages.json and the packages-lock.json files in your project...

Manually replacing 1.4.0 for 1.3.0, the packages-lock.json has to be reverted (I used git's history to locate the previous version to the upgrade) because it contains hashes of the 1.4.0 packages and these will not match the 1.3.0 versions.

after those files are updated (and commited) try again.

@skinny85
Copy link
Contributor

@carlosrfernandez thanks for posting that workaround, but I think I speak for everyone on the team when I say that experience does not meet our CDK usability bar :(

@carlosrfernandez
Copy link
Contributor

carlosrfernandez commented Aug 20, 2019

@skinny85 I was merely pointing out that it is possible to roll back. And in our case we had no alternative than to apply this workaround.

I am in total agreement with you that the desired developer experience should be much better.

@skinny85
Copy link
Contributor

skinny85 commented Aug 27, 2019

Some implementation notes/questions below.


So, I believe the correct way to model this in package.json is the following:

  1. dependencies should be pinned to an exact version; so, it should be "@aws-cdk/core": "1.5.0", not "@aws-cdk/core": "^1.5.0" as it is today.
  2. peerDepedencies should remain open to any compatible major version, the way it is today. That allows customers to try and use the core libraries with third-party libraries by changing the versions in their package.json. So, it should remain "@aws-cdk/core": "^1.5.0", not be changed to "@aws-cdk/core": "1.5.0" like in dependencies.

If that proposal seems OK to everybody, this requires changes in the following places:

  1. The JSII build tools assume dependency and peerDependency are identical. We would have to loosen this requirement; maybe just check that the dependencies version satisfies peerDependency version, like we do in pkglint in the CDK?
  2. We should also change the way versions of local dependencies are inferred in JSII (always with a caret).
  3. After those change in JSII, we would have to modify the sync-peer-deps.js script - probably also checking for dependencies version satisfying peerDependency version instead of a strict equality.

@eladb
Copy link
Contributor

eladb commented Aug 27, 2019

Just to make sure I understand the problem. It is only caused when users choose to pin their dependencies, correct? If they use a caret dependency, they should also get the latest compatible one.

EDIT: I think we still have an issue with transitive dependencies.

@KnisterPeter
Copy link
Contributor Author

The main problem are transitive dependencies. Right now the contract in cdk is to use the same minor version off all packages. With a carret this semantic is violated.
You may consider to use a tilde to get latest bugfixes on all transitive dependencies.

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 27, 2019

You may consider to use a tilde to get latest bugfixes on all transitive dependencies.

Still not good enough for intra-CDK dependencies. We might release a bug in any version (even a patch version), and with any ^ or ~ users won't have a way to roll back from that.

@KnisterPeter
Copy link
Contributor Author

Thats right, but with a bug in a bug-release another release should be easy and fast.
But sticking to fixed versions all along is also good enough.

@RomainMuller
Copy link
Contributor

The fix-peer-deps.js script shouldn't be needed anymore - jsii will do this automatically for you unless you ask it not to (by passing --no-fix-peer-dependencies).

@skinny85
Copy link
Contributor

PR in JSII: aws/jsii#747

@skinny85 skinny85 added bug This issue is a bug. and removed needs-triage This issue or PR still needs to be triaged. labels Aug 27, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 18, 2019

Let's research the following approach:

Reading on the spec that npm is trying to satsify:

https://yarnpkg.com/blog/2018/04/18/dependencies-done-right/

@artyom-melnikov
Copy link

Any updates on this issue? Inability to pin dependencies result in often conflicts at least in my team, not sure about others

@skinny85
Copy link
Contributor

Yes, we recently switched to having a pinned dependency on the CDK-internal modules: #4470 . That should make it easier to roll back to any version that was released after that change (1.13.0 or later).

@skinny85 skinny85 added the p0 label Oct 31, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 7, 2019

I think this particular issue doesn't need any more work right now. Closing.

@rix0rrr rix0rrr closed this as completed Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p0
Projects
None yet
Development

No branches or pull requests

7 participants