ca98am79 / connect-dynamodb

DynamoDB session store for Connect
http://ca98am79.github.com/connect-dynamodb/
MIT License
144 stars 66 forks source link

Update to V3 API #76

Closed lancegliser closed 1 year ago

lancegliser commented 1 year ago

Starting work on #71. Just a draft for visibility as I start hacking at it.

Changes (I'd suggest squash merging to these 😅)

lancegliser commented 1 year ago

@ca98am79 I'll try to keep to JS only, but the Types need some work too. Would you be adverse in the future to moving to tsc?

ca98am79 commented 1 year ago

@lancegliser that is fine with me, thank you

lancegliser commented 1 year ago

@ca98am79 The README currently has this warning:

  - `reapInterval` Legacy session expiration cleanup in milliseconds.  Should instead enable [DynamoDB TTL](http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/TTL.html) and select the `expires` field.  **BREAKING CHANGE** from v1.0.11 to v2.0.0 for reaping sessions with changes to the format of the expires field timestamp.

I've for now updated to:

- `reapInterval` Legacy session expiration cleanup in milliseconds.
  ☣️ Legacy reap behaviors use DynamoDB [scan](https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/clients/client-dynamodb/classes/scancommand.html)
  functionality that can incur significant costs. Should instead enable [DynamoDB TTL](http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/TTL.html)
  and select the `expires` field.

Since we're already making a breaking change, would it be better to remove that and help users avoid shooting themselves in the foot with potential costs? I should be able to use new UpdateTimeToLiveCommand() to apply a sensible default or take it as an option? Related to #72

lancegliser commented 1 year ago

@ca98am79 Alright my friend. I've got this to the point it works when manually npm packed into my project. I'm ready to have you review it.

A few things that might come as surprises I'll explain:

lancegliser commented 1 year ago

@ca98am79 We're online using this for a couple weeks now via npm pack install. Can I help in any way to get this merged and published?

lancegliser commented 1 year ago

@ca98am79 Noticed an issue today trying to upgrade from 16.x to 18.x on an AWS Lambda today. I traced it back to the this branch and the package I'd built from it. Wanted to use ScalarAttributeType.*, and it worked on 16.x. I assume 16.x just took the property as the value so ".N" came out correct. 18.x throws an error. I pushed and updated to fix it. Would love to get this published so we can properly version!

StefanTobler commented 1 year ago

Agreed, can we have this reviewed and merged. It is tough to have multiple mocked AWS clients across my different application with AWS sdk different versions.

ca98am79 commented 1 year ago

thank you, sorry for the delay