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

chore(deps): replace deprecated lib to alternative #2247

Merged
merged 5 commits into from
Mar 26, 2024

Conversation

yamatatsu
Copy link
Contributor

Description of your changes

This PR remove deprecated library @aws-sdk/util-base64-node replacing to @smithy/util-base64 that is official alternative library.
See, https://www.npmjs.com/package/@aws-sdk/util-base64-node.

Related issues, RFCs

Issue number: #2246

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my change is effective and works
  • The PR title follows the conventional commit semantics

Breaking change checklist

Is it a breaking change?: NO

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@yamatatsu yamatatsu requested review from a team as code owners March 18, 2024 13:27
@boring-cyborg boring-cyborg bot added parameters This item relates to the Parameters Utility dependencies Changes that touch dependencies, e.g. Dependabot, etc. tests PRs that add or change tests labels Mar 18, 2024
@pull-request-size pull-request-size bot added the size/S PR between 10-29 LOC label Mar 18, 2024
@dreamorosi dreamorosi added the do-not-merge This item should not be merged label Mar 18, 2024
@dreamorosi
Copy link
Contributor

Hi, thank you for taking the time to open this PR.

As I mentioned in the linked issue, we would like to go in a different direction and instead adopt the fromBase64 utility that we recently added to the commons package.

If you'd like to continue working on this and see the PR merged I'd kindly ask you to make changes in that regard and remove the @smithy dependency.

Apologies if this might cause you some rework, to avoid this type of occurrence in the future I'd advise to wait for a feedback from a maintainer in the issue before opening the PR, that way we'll be aligned from the beginning and we have a better chance to merge the PR with minor adjustments or as is.

@dreamorosi
Copy link
Contributor

Hi @yamatatsu, are you still interested in continuing this PR?

If not there's no problem at all and we'll take on the task at some point in the near future.

Please let me know, or we'll close the PR in a week.

@dreamorosi dreamorosi added the on-hold This item is on-hold and will be revisited in the future label Mar 24, 2024
@yamatatsu
Copy link
Contributor Author

@dreamorosi
Thank you for feedback. And so sorry for late response. 🙇🏻
I've addressed your comments.
In my thinking, we can depend on @smithy/xxx only as devDependencies. Or we should implement toBase64() like as fromBase64()? WDYT? 🤔

@dreamorosi
Copy link
Contributor

Hi @yamatatsu, no problem and thanks for coming back to this.

I agree with you, we can depend on it as a devDependency for now since we use toBase64() only in the tests.

Thanks!

Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

I have left one comment to fix the implementation and make the tests passing, once that's addressed we can merge this!

Copy link

sonarcloud bot commented Mar 26, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
3.9% Duplication on New Code

See analysis details on SonarCloud

@dreamorosi dreamorosi removed do-not-merge This item should not be merged on-hold This item is on-hold and will be revisited in the future labels Mar 26, 2024
@dreamorosi dreamorosi self-requested a review March 26, 2024 10:31
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this PR and following through with all the feedback!

Congrats 🎉

@dreamorosi dreamorosi merged commit 0fdabf4 into aws-powertools:main Mar 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Changes that touch dependencies, e.g. Dependabot, etc. parameters This item relates to the Parameters Utility size/S PR between 10-29 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: remove a dependency to deprecated library @aws-sdk/util-base64-node
2 participants