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

chore: update constructs dependency to v10.3.0 and jsii tools #12

Merged
merged 4 commits into from
Apr 22, 2024

Conversation

hoegertn
Copy link
Contributor

Bumps version of 'constructs' to 10.3.0 across different package files, including package-lock.json, .projen/deps.json, package.json, and explicitly sets constructsVersion in .projenrc.ts.

Updates dependencies related to jsii, including jsii-diff, jsii-docgen, jsii-pacmak, and their associated entries in the package-lock.json.

Also standardizes working-directory for GitHub Actions build workflows to './'.

These updates ensure compatibility with newer versions of constructs and jsii toolchain, enhancing project's build process and keeping dependencies up-to-date.

Fixes #

Bumps version of 'constructs' to `10.3.0` across different package files, including package-lock.json, .projen/deps.json, package.json, and explicitly sets `constructsVersion` in .projenrc.ts.

Updates dependencies related to jsii, including `jsii-diff`, `jsii-docgen`, `jsii-pacmak`, and their associated entries in the package-lock.json.

Also standardizes working-directory for GitHub Actions build workflows to './'.

These updates ensure compatibility with newer versions of constructs and jsii toolchain, enhancing project's build process and keeping dependencies up-to-date.
@hoegertn hoegertn enabled auto-merge April 14, 2024 14:07
@mrgrain
Copy link
Contributor

mrgrain commented Apr 14, 2024

Looks good to me. Given that we don't really release anything yet this is fine. Going forward, changing the minimum version of a peer dependency might be considered breaking my some; at the very least it should be marked as a feat

@hoegertn
Copy link
Contributor Author

I am unsure if a non-breaking dep update should be considered breaking. We would never be able to update the aws-cdk-lib version otherwise.

@mrgrain
Copy link
Contributor

mrgrain commented Apr 15, 2024

My personal view on this:

  • @open-constructs/aws-cdk-lib is at v1.0.0 and declares a peerDependency of aws-cdk-lib@^2.50.0.
  • @mrgrain/some-app takes a dependency on @open-constructs/aws-cdk-lib@^1.0.0
  • For some reason I need to fix my dependency on aws-cdk-lib to exaxctly [email protected]
  • @open-constructs/aws-cdk-lib releases v1.1.0 but now declares a peerDependency of aws-cdk-lib@^2.100.0.
  • If I'm updating packages in @mrgrain/some-app to use the latest version allowed by the defined version constraints, my app will now break. In the best case this is just a package manager error that can be silences, in the worst case the updated code now depends on a new API and will fail at runtime.

I'm aware this is maybe not the most popular opinion. There's plenty of online resources on this discussion (unrelated to CDK).

FWIW the reason why it's tempting to increate the minimum peerDep constraint is because JS don't have any reflection APIs to conditionally execute new code based on runtime decisions. We'd probably have to build a framework for it. The other alternative really is to have fixed release schedule for new major versions and always upgrade to the latest CDK version in those.

Anyway, I don't have high stakes in this.

@hoegertn
Copy link
Contributor Author

hoegertn commented Apr 15, 2024

I get your point but in your example that would not be our fault but the fault of the CDK team if 2.100 is a breaking change to 2.55

We need to upgrade to the "latest" CDK often to use the latest L1s

@moltar
Copy link
Contributor

moltar commented Apr 15, 2024

Updating a peer dep is most definitely a breaking change. This horse has been beaten do death on this issue, and it feels like that is the current consensus that peer change demands major bump.

Although, CDK project does not follow this rule for everything. Every -alpha. package gets a peer bump on every release (in sync with aws-cdk-lib) and yet the major is never bumped.

At the same time, I am of the opinion that keeping versions artificially low is not required. I don't know why devs get so attached to a major version. I say go ahead, bump it, and document what has changed. It's not a problem.

Renovate is on version 37.296.0 already, and they are the pros on versioning ;)

@mrgrain
Copy link
Contributor

mrgrain commented Apr 15, 2024

I get your point but in your example that would not be our fault but the fault of the CDK team if 2.100 is a breaking change to 2.55

We need to upgrade to the "latest" CDK often to use the latest L1s

There is any number of reasons why someone "needs" to fix their dependency to a lower version. The upstream team making a mistake is just one of them. It could also be due to internal process; it could be due to other dependencies; it could be due to workload (I don't have time to review every change that comes with the upgrade); it could be because I am accidentally relying on a bug in the old version.

I don't believe the reasons why someone chooses to do this should matter to us a library authors. We have a choice to make if we want to support this or not.

At the same time, I am of the opinion that keeping versions artificially low is not required. I don't know why devs get so attached to a major version. I say go ahead, bump it, and document what has changed. It's not a problem.

I think this comes from the worry that major version upgrades are (a lot of) manual work. If it's a minor version bump, you can probably automate this to some degree. For major versions people tend to not do this, because breaking changes are expected. But then again, this is mostly due to the owners of the packages. We also have a predictable yearly Node.js upgrade cycle and I don't really hear folk complaining about that. Probably because for the most of it these updates don't cause issues.

@hoegertn hoegertn requested a review from mbonig April 21, 2024 17:59
@hoegertn hoegertn added this pull request to the merge queue Apr 22, 2024
Merged via the queue into main with commit 02158a6 Apr 22, 2024
5 checks passed
@go-to-k go-to-k mentioned this pull request May 27, 2024
1 task
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.

4 participants