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

Specify aws-sdk only as peer dependency #36

Closed
wants to merge 1 commit into from

Conversation

specious
Copy link

If aws-sdk is declared as a peer dependency it probably makes little sense to also have it listed as a dev dependency.

@sonarcloud
Copy link

sonarcloud bot commented Jun 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@specious
Copy link
Author

Also, as a heads up, npm 8 automatically changes package-lock.json to the newer lockFileVersion: 2 format.

@Sleavely
Copy link
Owner

Since the behavior changed in npm v7 I'm not sure what the best approach would be anymore - I marked aws-sdk as a dev-dependency because it needs to be installed for development, but in a production environment I wouldn't want it to be installed automatically just because I install dynamo-plus; it is globally installed in the AWS Lambda environment and shipping an extra copy of the SDK would significantly increase the size of the function payload.

@specious
Copy link
Author

as a dev-dependency because it needs to be installed for development

That's exactly what I figured.

From the latest docs: "As of npm v7, peerDependencies are installed by default."

I wouldn't want it to be installed automatically just because I install dynamo-plus; it is globally installed in the AWS Lambda environment

Same thing.

I'm in the process of testing it out to see if I can use it in earthstewards/littermap-aws-backend, in particular because it might take care of batch requests.

@specious
Copy link
Author

Works like a charm.

@Sleavely
Copy link
Owner

Status update: I haven't forgotten this pull request, but I'm unsure if the proposed changes are the appropriate course of action to maintain the behavior I'd like.

I wouldn't want it to be installed automatically just because I install dynamo-plus; it is globally installed in the AWS Lambda environment

Same thing.

I don't think it is - if we continue to rely on aws-sdk in peerDependencies according to this PR in its current state it would mean that a workflow that runs npm install --production with npm 7+ to package a lambda would automatically include aws-sdk, significantly increasing the function size.

I've been procrastinating dealing with npm 7+ for a long time because from what I can tell they don't account for scenarios like this one where we explicitly do not want to install aws-sdk. I've been spending my morning trying to wrap my head around the discussion that caused the change and whether our usecase was even considered and it seems like Yarn and pnpm maintainers decided to stick with the old behavior as a result (although things may have changed in subsequent discussions or PRs since 2020).

I would prefer to keep aws-sdk in devDependencies since it is needed for development of dynamo-plus - and devDependencies of a dependency should AFAIK never be installed, so it wont affect consumers of dynamo-plus.

As far as peerDependencies go I think that the easiest backwardscompatible approach - albeit not pretty - is to simply omit it and instead rely on the README as a means of explaining why it is required at runtime but not part of the installation.

@Sleavely
Copy link
Owner

Sleavely commented Mar 8, 2024

I did eventually forget about this PR. Sorry about that!

With the upcoming 2.0 release there will be an explicit dependency on @aws-sdk/ v3 packages, so I'm closing this as outdated.

@Sleavely Sleavely closed this Mar 8, 2024
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.

2 participants