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

feat(src): enable types as peer dependencies to get around bumping wi… #35

Merged

Conversation

alexforsyth
Copy link
Contributor

Description

Changes aws-sdk-client-mock to use peer dependencies for AWS clients instead of pinned dependencies so that users can "bring their own" version. This fixes a bunch of type errors where typescript would expect the latest types without actually needing them. There's no reason that aws-sdk-client-mock needs to be updated with each and every single update to the AWS JS SDK v3.

This is the follow up to our action item from last week's meeting with @m-radzikowski

Similar PR for higher level libraries in aws-sdk-v3:
aws/aws-sdk-js-v3#2516

Testing

I've published this to my local verdaccio and tested that I can plug in any 3.x.x version. I'd appreciate another set of eyes here. Hopefully i'm not missing too much :).

Comment on lines +52 to +56
"@aws-sdk/types": "^3.18.0",
"@aws-sdk/client-dynamodb": "^3.18.0",
"@aws-sdk/client-sns": "^3.18.0",
"@aws-sdk/client-sqs": "^3.18.0",
"@aws-sdk/lib-dynamodb": "^3.18.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the latest versions updated for local tests. We may as well just pull in the latest for tests to run against those.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind that the ^ doesn't mean the "latest" will be installed when you run npm / yarn install. If you install dependencies today and a new version will be released tomorrow, running yarn install again will not install the fresh versions, since the matching ones are already installed.

This is one of the reasons why I prefer to specify exact dependency versions (in open source only dev dependencies ofc).

But let's keep it that way, I may just change it later.

"@aws-sdk/client-sqs": "3.18.0",
"@aws-sdk/lib-dynamodb": "3.18.0",
"@aws-sdk/types": "3.18.0",
"@aws-sdk/types": "^3.18.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Types is also the latest for just dev work.

@@ -45,12 +45,15 @@
"sinon": "^11.1.1",
"tslib": "^2.1.0"
},
"peerDependencies": {
"@aws-sdk/types": "^3.0.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should allow any aws-sdk/type above 3.0.0 that the customer wants to bring. This is should be resolved at a higher tree node when people npm install

@m-radzikowski
Copy link
Owner

Hey, thank you for the PR!

While peer dependency in fact looks like a way to go, I can't make it work locally.

Locally, I changed dev dependencies to 3.15.0, to imitate being on the older version of the SDK. In second project I added @aws-sdk/client-sns: 3.18.0 and the mock library with file:/... Did a clean yarn install in both.

As a result, I'm getting Client incompatibility error. It only works with @aws-sdk/client-sns: 3.15.0.

Maybe I'm doing something wrong. Can you double-check it on your side?

@alexforsyth
Copy link
Contributor Author

alexforsyth commented Jun 24, 2021

@m-radzikowski Are you publishing this to a local npm proxy? and then pulling from there? I use verdaccio. If you use yarn link or something like that it'll peer types badly

@alexforsyth
Copy link
Contributor Author

The dev deps shouldn't be exposed at all

@m-radzikowski
Copy link
Owner

Ok, hoped the yarn to be a little bit smarter. Will try the verdaccio, thanks!

Btw, please run yarn install and commit the updated yarn.lock file.

@alexforsyth
Copy link
Contributor Author

on it!

@codecov-commenter
Copy link

Codecov Report

Merging #35 (0bf981f) into main (8a597d3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #35   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines           77        77           
  Branches         9         9           
=========================================
  Hits            77        77           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a597d3...0bf981f. Read the comment docs.

Copy link
Owner

@m-radzikowski m-radzikowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works well and I learned something new. Thanks!

Comment on lines +52 to +56
"@aws-sdk/types": "^3.18.0",
"@aws-sdk/client-dynamodb": "^3.18.0",
"@aws-sdk/client-sns": "^3.18.0",
"@aws-sdk/client-sqs": "^3.18.0",
"@aws-sdk/lib-dynamodb": "^3.18.0",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind that the ^ doesn't mean the "latest" will be installed when you run npm / yarn install. If you install dependencies today and a new version will be released tomorrow, running yarn install again will not install the fresh versions, since the matching ones are already installed.

This is one of the reasons why I prefer to specify exact dependency versions (in open source only dev dependencies ofc).

But let's keep it that way, I may just change it later.

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.

3 participants