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 version 3 #325

Merged
merged 6 commits into from
Jul 14, 2024
Merged

Conversation

alice-was-here
Copy link

This should resolve this issue: #241

I have used a variation of @Sljux's solution to make the interface compatible with the current stream interface.

I've chosen to make the v3 interface a completely separate method... the two clients are different enough that I think it makes sense to break them into two different paths. I can merge them under one function call if people would prefer that.

This should resolve this issue: ZJONSSON#241
I have used a variation of @Sljux's solution to make the interface compatible with the current `stream` interface.
lib/Open/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ZJONSSON
Copy link
Owner

ZJONSSON commented Jul 6, 2024

Thanks @alice-was-here, appreciate the PR. Can you please address the comments above and also add a unit test for the new s3 method, you can base the test on current s3 test https://github.com/ZJONSSON/node-unzipper/blob/master/test/openS3.js

@ZJONSSON
Copy link
Owner

ZJONSSON commented Jul 6, 2024

@alice-was-here Also can you please ensure tests pass for all node versions supported 10.x - 19.x
see test failures here: https://github.com/ZJONSSON/node-unzipper/actions/runs/9657965974/job/27093402969?pr=325
It is ok to skip s3_v3 unit tests for node versions that don't work with the v3 aws sdk.

lib/Open/index.js Outdated Show resolved Hide resolved
@ZJONSSON
Copy link
Owner

ZJONSSON commented Jul 8, 2024

@alice-was-here, this is super close, can you please address the following so we can get this in:

  • Fix the ?? operator so node v10.x will run tests successfully
  • Make sure that /lib/Open/index.js only includes your additional lines of code, please do not reformat the pre-existing code.
  • Add specific unit test for s3_v3. You may have to check for the node version and conditionally skip the test for node versions that do not work with @aws-sdk/client-s3
  • Before final submission pls ensure that 10x, 12.x, 14x, 16.x, 17.x, 18.x and 19.x node versions pass all unit tests

Here is a quick example of how this should look (passes tests): https://github.com/ZJONSSON/node-unzipper/pull/329/files - but we still need the additional unit test for s3_v3

@alice-was-here alice-was-here marked this pull request as draft July 9, 2024 00:57
@alice-was-here
Copy link
Author

@ZJONSSON cheers 🙏

I noticed that the tests for the existing s3 client are marked as skip - I was going to base my implementation on them, but when enabled, they also fail.

@jtinbergen
Copy link

I am only leaving a comment to say that aws-sdk v3 support is actually desired and we are excited to see someone write support for this.

"dirdiff": ">= 0.0.1 < 1",
"eslint": "^9.2.0",
"globals": "^15.2.0",
"iconv-lite": "^0.4.24",
"request": "^2.88.0",
"stream-buffers": ">= 0.2.5 < 1",
"tap": "^12.7.0",
"tap": "^16.3.10",
Copy link
Author

@alice-was-here alice-was-here Jul 10, 2024

Choose a reason for hiding this comment

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

Bumping the tap cli was required to correctly import the aws libraries (they include ?? and other syntax not supported by tap@12).

The coverage report also seems to compute differently for tap 16 - I've disabled reduced it to make the test suite pass.... it was computing coverage at about 80% even though nothing else had changed. If anyone has ideas as to why it dropped off, happy to add back?

// These files are provided by AWS's open data registry project.
// https://github.com/awslabs/open-data-registry

return unzip.Open.s3_v3(client, {
Copy link
Author

@alice-was-here alice-was-here Jul 10, 2024

Choose a reason for hiding this comment

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

If there's a better file source for this I can change it. The one referenced by the openS3.js test no longer exists.

@alice-was-here alice-was-here marked this pull request as ready for review July 10, 2024 13:54
@ZJONSSON
Copy link
Owner

Thanks again @alice-was-here , can you follow up with an update to README.md?

@ZJONSSON ZJONSSON merged commit d358d58 into ZJONSSON:master Jul 14, 2024
9 checks passed
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.

3 participants