aws / aws-sdk-js

AWS SDK for JavaScript in the browser and Node.js
https://aws.amazon.com/developer/language/javascript/
Apache License 2.0
7.59k stars 1.55k forks source link

Are `scripts` & `dist-tools` required in the published package? #3159

Closed G-Rath closed 4 years ago

G-Rath commented 4 years ago

Confirm by changing [ ] to [x] below:

Describe the question

I submitted a PR to add .vscode & .eslintignore to the .npmignore since they're not needed in the package.

I suspect that the scripts & dist-tools folders don't need to be published as well, but don't know the package well enough to make a PR.

These folders together take up about ~100kb on disk - while that might not seem like a lot, it adds up pretty quickly if you consider that's there for every release, which is stored in multiple caches which are accessed by CIs doing automatic builds that are triggered who knows how many times a minute; not to mention AWS itself has this package baked into its infrastructure countless times over, so it adds up to a decent savings for something that costs nothing :)

ajredniwja commented 4 years ago

Hey @G-Rath, thank-you for reaching out and bringing this up,

scripts is used by some of the internal customers and dist-tools is used for browser tests.

That being said I think that dist-tools can be removed from the package but not sure about the customer impact for scripts. You can submit a similar PR and I can have more inputs from the team.

Marking it as a feature request. Please reach out if you have any additional questions.

G-Rath commented 4 years ago

Awesome, thanks for the response. I'll make two PRs so they don't block each other :)

matthopson commented 4 years ago

We're seeing build failures this morning in CircleCI while running Cypress tests with the following error on aws-sdk 2.643.0:

Error: Cannot find module './dist-tools/transform.js' from '/home/app/node_modules/aws-sdk'

Are we sure it was safe to remove that, or are is this potentially something we are doing wrong on our end?

AllanZhengYP commented 4 years ago

Hey @matthopson Can you find how ./dist-tools/transform.js is imported to your project? dist-tools folder contains tools we use to manage our release, it shouldn't be consumed directly.

matthopson commented 4 years ago

@AllanFly120 Yeah, I was trying to work that out. We downgraded the previous version and the problem went away. There is at least one other case I'd seen on SO (posted this morning).

The first place that jumps out to me is here: https://github.com/aws/aws-sdk-js/blob/2f5261f46eb95a33ddb72ba27a6f7304e4f9ded9/package.json#L62

Given the following error (more context than what I originally posted):

The error was:

Error: Cannot find module './dist-tools/transform.js' from '/home/app/node_modules/aws-sdk'

This occurred while Cypress was compiling and bundling your test code. This is usually caused by:

- A missing file or dependency
- A syntax error in the file or one of its dependencies

Fix the error in your code and re-run your tests.

We're not doing anything earth-shattering here: https://github.com/flexion/ef-cms/blob/develop/web-api/smoke-tests.js

And it breaks at this latest version, and not prior.

AllanZhengYP commented 4 years ago

@matthopson Good catch I see where the problem is. Will fix it shortly!

ajredniwja commented 4 years ago

@matthopson fixed in #3178

ajredniwja commented 4 years ago

@G-Rath as observed, this will be a breaking change for many customers, hence would like to close this.

G-Rath commented 4 years ago

@ajkerr fair enough, but you can still exclude dist-tools/tests.

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.