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

Add support for @aws-sdk/client-s3 #277

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

Conversation

alvin-nt
Copy link

@alvin-nt alvin-nt commented May 10, 2023

This is an attempt to add support for @aws-sdk/client-s3 (AWS SDK v3). If merged, this PR should resolve #241

refactor: use promise to resolve stream
@alvin-nt alvin-nt changed the title Add rudimentary support for @aws-sdk/client-s3 Add support for @aws-sdk/client-s3 May 10, 2023
else
resolve(d.ContentLength);
if (isAwsSdkV3) {
return client.headObject(params).then(function (res) {
Copy link

@zandaqo zandaqo Jun 13, 2023

Choose a reason for hiding this comment

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

But AWS SDK V3 doesn't have headObject function? S3Client doesn't have the said methods, S3 class does, though, it doesn't have the send method, hence, the PR still breaks.

Copy link

@ChrisZieba ChrisZieba Jun 26, 2023

Choose a reason for hiding this comment

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

hmm, yea I gave this a try and it doesn't seem to work out. What if you did something like

try {
   return client.headObject(params).promise()
} catch(e) {
   if (e.msg === 'something') {
        return client.headObject(params).then()
   }
}

Choose a reason for hiding this comment

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

@alvin-nt thanks for opening this PR, do you have the bandwidth to add this tweak or should I give it a shot?

Copy link
Author

Choose a reason for hiding this comment

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

Hi. Apologies for the long reply. Due to life circumstances, I could not continue this PR yet. If you may, please give a shot to fix my code.

Copy link

Choose a reason for hiding this comment

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

Wouldn't it be enough to change isAwsSdkV3 to this:

            const isAwsSdkV3 = typeof client.send === "function";

Here, at least, it gives true with v3 and false with v2

Copy link

Choose a reason for hiding this comment

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

Hi, Maybe we can do something like:

    if (isAwsSdkV3) {
          return client.send(new HeadObjectCommand(params)).then(function (res) {
            return res.ContentLength;
          });
    }

@ZJONSSON
Copy link
Owner

ZJONSSON commented Jun 8, 2024

Thanks @alvin-nt and sorry for the long delay. I'm being more active in managing this repo and your PR is now deviating from master. Can you rebase and clean it up so it's only reflecting the s3 change and no other changes

Thanks again!

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.

Feature request: support for aws-sdk v3
6 participants