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

Make aws-sdk a peer dependency #162

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

johnhaley81
Copy link

Much like joi, aws-sdk is often required by both the library and the
consumer so it also makes sense to be a peer dependency as well.

Much like joi, aws-sdk is often required by both the library and the 
consumer so it also makes sense to be a peer dependency as well.
@coveralls
Copy link

coveralls commented Jul 23, 2018

Coverage Status

Coverage increased (+0.2%) to 98.725% when pulling e203c0f on johnhaley81:make-aws-sdk-peer-dep into df7acfe on clarkie:master.

@ntanitime
Copy link

Let's do it 👍

@simlu
Copy link

simlu commented Sep 12, 2018

What's the hold up?

package.json Outdated
@@ -45,6 +45,7 @@
"sinon": "4.0.1"
},
"peerDependencies": {
"aws-sdk": "2.131.0",
Copy link

Choose a reason for hiding this comment

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

This should not be pinned (!)

Copy link
Author

Choose a reason for hiding this comment

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

I agree, but it was pinned before so I kept it the same.

Copy link

Choose a reason for hiding this comment

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

I would strongly recommending changing it. Dependency pinning makes perfect sense, however it doesn't make sense for peer dependencies (especially not for a fast changing one like aws sdk). It defeats the purpose plus this dependency has a known vulnerability.

Copy link
Author

Choose a reason for hiding this comment

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

Ahhh ok great point. I'll change it.

@dremonkey
Copy link

Why hasn't this been merged yet?

@cdhowie
Copy link
Collaborator

cdhowie commented Mar 29, 2019

I can merge it but I'm not sure what good it would do; @clarkie doesn't seem to be around anymore and I don't have the ability to push releases to npm.

@simlu
Copy link

simlu commented Mar 29, 2019

@cdhowie maybe it's time to fork and publish under a different package name?

@simlu
Copy link

simlu commented Mar 29, 2019

Having said that... I'm in the process of writing my own orm. So I'm don't really care about this project much anymore (sorry).

@dremonkey
Copy link

@cdhowie @simlu Thanks for the updates. Good to know.

@simlu Be interested in seeing what you come up with. When's that coming out?

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.

None yet

6 participants