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.

Sign up for GitHub

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

Jump to bottom

AWS JavaScript SDK V3 #162

Merged
merged 12 commits into from
May 27, 2022
Merged

AWS JavaScript SDK V3 #162

merged 12 commits into from
May 27, 2022

Conversation

Takaitra
Copy link
Contributor

@Takaitra Takaitra commented Jan 10, 2022 •
edited

Description

Addresses #146

AWS SDK for JavaScript, v3, a rewrite of the AWS SDK, was announced in December 2020: https://aws.amazon.com/blogs/developer/modular-aws-sdk-for-javascript-is-now-generally-available/. V2 was monolithic whereas V3 introduces separate packages for each service. This upgrades multer-s3 to use v3 of the SDK, importing only the S3 module. This will accompanied by a major version bump to 3.0.0.

Changes

  1. AWS SDK v3 multipart, streaming uploads via @aws-sdk/lib-storage
  2. Update tests
  3. Update README

Testing

  1. Unit tests via npm test
  2. Integration test via https://github.com/Takaitra/multer-s3-test

@Takaitra Takaitra mentioned this pull request Jan 10, 2022
Closed
@Takaitra
Copy link
Contributor Author

@LinusU can you please take a look? There is a lot of interest in a release that supports v3 of the AWS SDK.

@LinusU
Copy link
Collaborator

LinusU commented Jan 17, 2022

Use Multer's built-in mime type recognition for multerS3.AUTO_CONTENT_TYPE

Multer (2.x) doesn't have built-in mime type recognition, the value that code is updated to is the one provided by the client. Generally, depending on a mime type submitted by the client isn't a good idea and could lead to exploits (although detecting it isn't a silver bullet either, and could have similar problems...)

Anyhow, I not sure that we should remove that functionality since people might be using it? And if we are, we should update the documentation and remove the AUTO_CONTENT_TYPE constant.

Besides that, I would recommend to use Multer 3.x instead which makes this library obsolete...

@Takaitra
Copy link
Contributor Author

Takaitra commented Jan 18, 2022 •
edited

Besides that, I would recommend to use Multer 3.x instead which makes this library obsolete...

I'm having trouble finding Multer 3.x. Can you provide a link please?

depending on a mime type submitted by the client isn't a good idea

I'll look into reverting this part of the PR.
Done. The diff is smaller now.

LinusU
LinusU reviewed Jan 18, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
LinusU
LinusU reviewed Jan 18, 2022
View reviewed changes
package.json Outdated Show resolved Hide resolved
LinusU
LinusU reviewed Jan 18, 2022
View reviewed changes
package.json Outdated Show resolved Hide resolved
LinusU
LinusU reviewed Jan 18, 2022
View reviewed changes
test/basic.js Outdated Show resolved Hide resolved
LinusU
LinusU reviewed Jan 18, 2022
View reviewed changes
test/util/mock-s3.js Outdated Show resolved Hide resolved
LinusU
LinusU reviewed Jan 18, 2022
View reviewed changes
index.js Outdated Show resolved Hide resolved
LinusU
LinusU reviewed Jan 18, 2022
View reviewed changes
Copy link
Collaborator

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Left a few comments, could you also please remove the package-lock.json file? Since this is a library for others to consume, that lock file will not be respected when anyone depends on this library and thus we don't want it committed here...

@LinusU
Copy link
Collaborator

LinusU commented Jan 18, 2022

I'm having trouble finding Multer 3.x. Can you provide a link please?

expressjs/multer#399

Takaitra and others added 4 commits January 18, 2022 08:59
@Takaitra @LinusU
Apply suggestions from code review
9061639 
Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>
@Takaitra @LinusU
Use callbackify
6a4dabc 
Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>
@Takaitra
Import util
4645590
@Takaitra
Remove package-lock.json
79eff37
@Takaitra
Copy link
Contributor Author

Takaitra commented Jan 18, 2022 •
edited

Thank you for the comments. I reran the unit and integration tests and they pass.

Regarding versioning, 2.x.x should be branched so it can continue to be maintained separately right?

@Takaitra Takaitra requested a review from LinusU January 18, 2022 17:13
@Takaitra
Copy link
Contributor Author

Bump

@aitorosabrooktec
Copy link

When can we expect merging of PR?

@1valdis
Copy link

1valdis commented Mar 25, 2022

@LinusU is there anything blocking the merge and release?

@lukes00
Copy link

lukes00 commented Apr 4, 2022

@LinusU would really appreciate this PR being merged 🙏

@adriandersen
Copy link

Any status on this?

@LinusU LinusU merged commit 92f1494 into anacronw:master May 27, 2022
LinusU pushed a commit that referenced this pull request May 27, 2022
@Takaitra @LinusU
💥 Upgrade to AWS JavaScript SDK V3 (#162)
e951e48 
Migration Guide:

When upgrading to this version, you also need to migrate from version 2 of the AWS JavaScript SDK to version 3. Instead of passing an instance of the old `aws.S3`, use the new `S3Client` class from `@aws-sdk/client-s3`.

The minimum required version of Node.js has also been increased to 12.0.0, to align with the AWS JavaScript SDK.
@LinusU
Copy link
Collaborator

LinusU commented May 27, 2022

I'm really sorry for dropping the ball on this!

It's now merged and released as 🚢 3.0.0 / 2022-05-27

@Takaitra Takaitra mentioned this pull request Jun 6, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LinusU LinusU Awaiting requested review from LinusU

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Takaitra @LinusU @aitorosabrooktec @1valdis @lukes00 @adriandersen