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

feat(NODE-4721): add aws-sdk as optional dependency #3446

Merged
merged 12 commits into from Oct 18, 2022
Merged

feat(NODE-4721): add aws-sdk as optional dependency #3446

merged 12 commits into from Oct 18, 2022

Conversation

durran
Copy link
Member

@durran durran commented Oct 13, 2022

Description

Uses the AWS-SDK for finding credentials when present in dependencies, otherwise uses the internal implementation.

What is changing?

  • Adds @aws-sdk/credential-providers as an optional dependency.
  • Uses the SDK when installed to find credentials from the environment when missing.

Patch build with AWS tests for with and without the optional dependencies:

https://spruce.mongodb.com/version/634d68f92fbabe2d0a6b964f/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

Is there new documentation needed for these changes?

Can make a note to users that they can install the SDK to have it be used.

What is the motivation for this change?

NODE-4721

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@durran durran marked this pull request as ready for review October 17, 2022 13:44
{
func: 'install dependencies',
vars: {
NPM_OPTIONS: '--no-optional'
Copy link
Member Author

Choose a reason for hiding this comment

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

We run the aws tests twice each now, once with the sdk installed and once without.

@@ -308,7 +308,7 @@ const AWS_AUTH_TASKS = [];

for (const VERSION of AWS_AUTH_VERSIONS) {
const name = ex => `aws-${VERSION}-auth-test-${ex.split(' ').join('-')}`;
const aws_funcs = [
const awsFuncs = [
Copy link
Member Author

Choose a reason for hiding this comment

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

Just changing name to JS conventions.

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

LGTM, I think we have really good coverage here, thanks for the CI adjustments. I think we will want to put a note about this in the Manual docs, so I did mark the ticket as docs changes needed.

@nbbeeken nbbeeken added the Team Review Needs review from team label Oct 17, 2022
@nbbeeken nbbeeken merged commit b879cb5 into main Oct 18, 2022
@nbbeeken nbbeeken deleted the NODE-4721 branch October 18, 2022 14:27
@avaly
Copy link
Contributor

avaly commented Oct 20, 2022

The install size of the mongodb package doubled in size in the last version: https://packagephobia.com/result?p=mongodb

Mostly because of this new dependency being added.

Can we please remove it from this package and instruct users to add this dependency in their project, if needed?

EDIT: yarn v3 does not support ignoring optional dependencies, so we're installing all those optional packages.

@avaly
Copy link
Contributor

avaly commented Oct 20, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
3 participants