Remove options that are provided by the AWS SDK

jeremydaly / data-api-client

A "DocumentClient" for the Amazon Aurora Serverless Data API

MIT License

439 stars 61 forks source link

Remove options that are provided by the AWS SDK #35

Closed jackstevenson closed 4 years ago

jackstevenson commented 4 years ago

Fixes https://github.com/jeremydaly/data-api-client/issues/33

The keepAlive option is better served by setting the environment variable AWS_NODEJS_CONNECTION_REUSE_ENABLED=1 and removes the need to configure a custom Agent.
The sslEnabled option is simply passed straight through to the underlying RDSDataConnection anyway so doesn't add anything.
The region option can also be set by passing a value using the options param, and probably shouldn't be setting the global region any way.

jeremydaly commented 4 years ago

I really like this line of thinking, and totally agree for the region and sslEnabled bits. I wonder if (since I'd rather it be on by default) we automatically set the AWS_NODEJS_CONNECTION_REUSE_ENABLED=1 environment variable and then give users the keepAlive option to disable it?

jackstevenson commented 4 years ago

I'm not sure library code should be setting environment variables as it could have unintended side effects in application code and takes control away from the user. I think it would be safer to configure custom agents in this case.

jackstevenson commented 4 years ago

@jeremydaly how would you like to proceed with this PR?

AndrewBarba commented 4 years ago

@jackstevenson @jeremydaly Any thoughts on merging this? I think it's a really important change. I actually disable keepAlive in the constructor specifically so the custom aws agent I provide is not overwritten. I don't think this library should be responsible for setting that, it could have really bad side effects if someone is already setting a custom global agent.

jeremydaly commented 4 years ago

Sorry for the very long delay on getting to this. I've removed the HTTP KeepAlive workaround, but kept the sslEnabled and region options for backwards compatibility. Both of those settings just manipulate the options object before being passed into the SDK, so they are effectively just shorthands for now.

jackstevenson commented 4 years ago

Thanks for picking this up again @jeremydaly