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

Peer dependencies are never direct #2170

Merged
merged 2 commits into from
Jan 18, 2024
Merged

Conversation

mxdvl
Copy link
Contributor

@mxdvl mxdvl commented Jan 8, 2024

What does this change?

Make the following direct dependencies that are already listed as peers, dev: aws-cdk-lib, constructs. This matches the setup of aws-cdk.

How to test

Install the package somewhere.

How can we measure success?

Declaring peer dependencies that are also direct ones has no value

Have we considered potential risks?

Consumers should ensure that they have the correct peer dependencies

Checklist

  • I have listed any breaking changes, along with a migration path 1
  • I have updated the documentation as required for the described changes 2

Footnotes

  1. Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs.

  2. If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid?

Copy link

changeset-bot bot commented Jan 8, 2024

🦋 Changeset detected

Latest commit: ee87cff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@guardian/cdk Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mxdvl mxdvl force-pushed the mxdvl/correct-peer-dependencies branch from 72b91ab to 5752b08 Compare January 8, 2024 15:58
@mxdvl mxdvl added the dependencies Pull requests that update a dependency file label Jan 8, 2024
@akash1810
Copy link
Member

Declaring peer dependencies that are also direct ones has no value

This package uses peerDependencies as a way to dictate which version of AWS CDK must be used by consumers of GuCDK.

If a different version of aws-cdk, aws-cdk-lib, or constructs is used, some fairly cryptic stack traces are seen1, similar to:

    lib/my-app.test.ts:9:29 - error TS2345: Argument of type 'Stack' is not assignable to parameter of type 'Stack'.
      Type 'Stack' is missing the following properties from type 'Stack': _suppressTemplateIndentation, _terminationProtection, toYamlString

    9   expect(Template.fromStack(stack).toJSON()).toMatchSnapshot();

Use of peerDependencies produces a warning at installation time when mismatched versions of used. The idea is that message provides a way to triage the above issue.

If there's a better way describe this in the package.json that'd be amazing! I think the changes in this PR break the above description.

Footnotes

  1. The are a few Chat threads sharing confusion on this stack trace.

@mxdvl
Copy link
Contributor Author

mxdvl commented Jan 15, 2024

Declaring peer dependencies that are also direct ones has no value

This package uses peerDependencies as a way to dictate which version of AWS CDK must be used by consumers of GuCDK.

Yes, that’s a correct definition of what peer dependencies are meant to do. However, I’m not sure what specifying a dependency as both peer and direct achieves. It’s not a defined behaviour in the docs. Peer dependencies are automatically installed by NPM since v7, so there’s no need to specify it as a direct one to install it.

@akash1810
Copy link
Member

akash1810 commented Jan 16, 2024

If a different version of aws-cdk, aws-cdk-lib, or constructs is used, some fairly cryptic stack traces are seen

https://github.com/guardian/service-catalogue/pull/680/files for a real-life example (see the added annotations).

@mxdvl mxdvl force-pushed the mxdvl/correct-peer-dependencies branch from 57e1354 to 4080032 Compare January 17, 2024 14:04
@mxdvl mxdvl requested a review from akash1810 January 17, 2024 14:06
mxdvl and others added 2 commits January 18, 2024 09:14
there’s no value in having the same dependency
declared as both direct and peer
@mxdvl mxdvl force-pushed the mxdvl/correct-peer-dependencies branch from 4080032 to ee87cff Compare January 18, 2024 09:14
@mxdvl mxdvl enabled auto-merge (squash) January 18, 2024 09:15
@mxdvl mxdvl merged commit 8ead267 into main Jan 18, 2024
2 checks passed
@mxdvl mxdvl deleted the mxdvl/correct-peer-dependencies branch January 18, 2024 09:18
@akash1810
Copy link
Member

Just to note this change follows the advice in https://docs.aws.amazon.com/cdk/v2/guide/manage-dependencies.html:

If you're developing a construct library, specify its dependencies via a combination of the peerDependencies and devDependencies sections

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants