opensearch-project / opensearch-js

Node.js Client for OpenSearch
https://opensearch.org/docs/latest/clients/javascript/
Apache License 2.0
186 stars 121 forks source link

adds a new export from the package for aws-sdk v3 #758

Closed tusharf5 closed 5 months ago

tusharf5 commented 5 months ago

Description

Based on the conversation in this issue #757 , this PR adds a new way to import the AwsSigv4Signer as shown below.

const { AwsSigv4Signer } = require('@opensearch-project/opensearch/aws-v3');

By using a new import path, the sdk remains backwards compatible while also adding a way for clients to avoid getting the entire aws-sdk included in the final bundle which results in an additional 22MB.

I tested the code and it does reduce the bundle size with the new import path

image

The bundle size is still not good though because '@aws-sdk/credential-provider-node' dynamically imports all types of credential providers ini, ec2, environment, node, process, http etc which is about 1MB in size.

Would it make sense to have the client provide the provider based on the environment instead of relying on the default provider chain? This would be a breaking change but it would delegate the credential responsibility on the client eliminating the dependency on aws-sdk & @aws-sdk/credential-provider-node?

https://github.com/opensearch-project/opensearch-js/blob/main/lib/aws/AwsSigv4Signer.js#L74

We will have to throw an error on this (making it required) line so the client always provides a getCredential function and we can remove the dependency on a default provider (both v2 and v3) from our end.

The doc examples already use the getCredential function so I am guessing most of the clients would already be providing this function and not relying on the default getCredential.

https://opensearch.org/docs/latest/clients/javascript/index/#authenticating-with-amazon-opensearch-service-aws-signature-version-4

Issues Resolved

Closes #757

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dblock commented 5 months ago

Would it make sense to have the client provide the provider based on the environment instead of relying on the default provider chain? This would be a breaking change but it would delegate the credential responsibility on the client eliminating the dependency on aws-sdk & @aws-sdk/credential-provider-node?

It's a good idea, but I would explore this in a future/separate PR.

tusharf5 commented 5 months ago

@dblock updated the PR with the requested changes

dblock commented 5 months ago

@tusharf5 fix CI pls, there's some license headers missing, and a complaint about a new license, 0BSD. It's just a BSD variant ~so I think we can add it to the list, but I'll go double check~ and we already allow it in OpenSearch-Dasbhoards.

dblock commented 5 months ago

The license workflow needs to have 0BSD added to it, https://github.com/opensearch-project/opensearch-js/actions/runs/8739773216/job/23986128639?pr=758.

tusharf5 commented 5 months ago

@dblock fixed

tusharf5 commented 5 months ago

checking the test error

tusharf5 commented 5 months ago

1 existing test was failing since aws-sdk and v3 ask were not installed so the error thrown was different and now they are part of dev dependency so the error has changed. I've updated the test. hopefully no more test failures. @dblock

Edit - For some reason I can't run tests locally, I get "ERR_REQUIRE_ESM" errors otherwise I would have caught & fixed these errors locally.

tusharf5 commented 5 months ago

Had to remove the peerDependency change since npm automatically installs peerDepenencies (yarn does not) and that changes the import result which fails the test. It should be working now.