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

Could we move aws-sdk to devDeps? #144

Closed
tabroughton opened this issue Mar 11, 2021 · 11 comments
Closed

Could we move aws-sdk to devDeps? #144

tabroughton opened this issue Mar 11, 2021 · 11 comments

Comments

@tabroughton
Copy link

I'm not sure of all the use cases but I assume that this is largely to run on aws lambda and the aws-sdk is already there.

With it being a prod dependency then my --production packages that depend on winston-cloudwatch are too large for lambda@edge and I have to remove winston-cloudwatch and I've ended up just using console.log as a work around for now.

If we moved aws-sdk to devDependencies then I wouldn't have this issue and others who need to deploy the aws-sdk in their production package that uses winston-cloudwatch could define it in their own package (though I'm curious why that would be needed).

@tabroughton
Copy link
Author

...Almost 10MB with the current deps. Is there a reason that the full aws-sdk needs to be included?

@lazywithclass
Copy link
Owner

lazywithclass commented Apr 1, 2021

Hi @tabroughton, I honestly do not remember why I ended up having aws-sdk as a dependency, I recall there was some talk here in the issues; as you can see the project is quite old and I stopped using it in production several years ago, I am just trying to maintain it now.

I can't say for sure if the vast majority of people using this are on AWS Lambda; anyway, when you say "though I'm curious why that would be needed", what do you mean? What's the intended way? I'm genuinely asking since I haven't had the chance to work with node since years, and now there might be more robust solutions.

@tabroughton
Copy link
Author

can't say for sure if the vast majority of people using this are on AWS Lambda; anyway, when you say "though I'm curious why that would be needed", what do you mean? What's the intended way? I'm genuinely asking since I haven't had the chance to work with node since years, and now there might be more robust solutions.

I mention about the aws-sdk because AWS Lambdas already have the aws-sdk installed so there's no need to have it listed as a dependency in for production in the package.json, if I do then I when I build my --production app to deploy to lambda I have a very large application to deploy. Too large for lambda@edge and also makes my normal lambdas very large.

But if you take it out of the package.json and put it in the devDependencies instead then when I install it for production it wouldn't have the aws-sdk installed in the node_module. (the dependency is already on the lambda so that's fine).

Only problem I can see are those folks using it in other environments such as EC2s they manage themselves and docker containters in ECS. When they package and deploy they might find they had the dependency missing, they would have to include aws-sdk alongside winston-cloudwatch as a dependency in whatever application they're deploying. It means they would have to change their code, your latest package wouldn't be backward compatible. - but I think that's acceptable if there were a mention on it on your readme.

@lazywithclass
Copy link
Owner

I see, first of all thank you for the detailed explanation. As of now I think it's like a short blanket, either way someone's feet are going to be cold :D

I'll think about this and see if there are possibile solutions, even maybe two separated builds, I'm not sure, but I'll give this some thoughts.

@solarisn
Copy link

@lazywithclass I think the best solution is to replace the aws-sdk dependency with @aws-sdk/client-cloudwatch-logs. Version 3 of the AWS SDK for Javascript is modular and enables installation of independent service packages. This will greatly decrease the size of the dependency and the upgrade process is rather straightforward.

@kevin-king
Copy link

kevin-king commented Jul 12, 2021

+1

Encountered this on a project. AWS SAM duplicates your lambda NPM modules for every lambda. winston-cloudwatch adds an additional 60 MB per lambda due to the aws-sdk as a regular dependency as @tabroughton described.

This is especially problematic for two reasons (1) it's a logging library, so it's very common that most/every lambda needs winston-cloudwatch (2) it's an issue that scales with your service size. Currently our service is sitting at 17 lambdas and this library causes over 1 GB artifact size (It would be about 20 MB in total otherwise, for reference).

Edit: I also tried using winston-aws-cloudwatch as an alternative library, but it also has aws-sdk independencies.

@lazywithclass
Copy link
Owner

lazywithclass commented Jul 16, 2021

I will have some time over the weekend and will try to add this to the module, as I understand the size has become a problem for a lot of you!

I just need to understand how to preserve the previous behaviour, for example how to pass a proxy server to the config options.

Sadly the docs do not come with an example or deep explanation.

@kevin-king
Copy link

kevin-king commented Jul 16, 2021

Much appreciated @lazywithclass! Right now we've gotten around the issue by being selective in what we copy into our SAM artifact with a negative glob:

!.aws-sam/**/node_modules/aws-sdk/**
!.aws-sam/**/node_modules/@types/**

Which works pretty well since it guards against this issue from all dependencies.

In an earlier solution, that we didn't go with, I created a fork of this repo, moved aws-sdk to devDependencies, and got the result that I was looking for (reduced artifact). I realize there's probably more to the puzzle, like you mentioned the config options, but just a vote of confidence.

@lazywithclass
Copy link
Owner

lazywithclass commented Jul 16, 2021

I gave this a look, they changed pretty much everything, for example the way I would have to deal with PutLogEventsCommand and all the new *Command functions.
This module has aged really poorly and bears all the scars of the previous unexperienced me, for example in the way I passed around instances of cloudwatchlogs.
With this change and without an almost complete rewrite of all involved functions the module would be in an even more sorry state that it is now.

tl;dr I am moving aws-sdk to devDependencies (as some of you suggested), add a notice about this in the README, then publish 3.0.0 version, with also an update of all dependencies.

@lazywithclass
Copy link
Owner

winston-cloudwatch@3.0.0 has aws-sdk into devDependencies.

Please let me know if you have any troubles with that.

@msreekm
Copy link

msreekm commented Jan 6, 2022

i am seeing this error when runnning in docker - Error: Cannot find module 'aws-sdk'. could it be related to moving to dev-dependency ? is aws-sdk required to run this in production build? if so, why is it dev-dependency? so is the recommendation is to include aws-sdk separately ? i am assuming aws-sdk is used to send to cloudwatch which is the main purpose of this library?

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

No branches or pull requests

5 participants