mapbox / dyno

simple dynamodb client
MIT License
78 stars 28 forks source link

Rely on aws-sdk #106

Closed rclark closed 8 years ago

rclark commented 9 years ago

Full-blown refactor with some of the ideas laid out in #105. Still needs:

cc @mick @willwhite

rclark commented 8 years ago

Here is some detail on the changes this PR would inflict. The overarching goal is to prefer to utilize the aws-sdk commands where possible, inheriting their bug fixes, updates, and documentation. Some aspects of dyno's API are really nice (e.g. .getItem(key) instead of .getItem({ Key: key })), but having documentation that we don't have to maintain is arguably better.

generally speaking

... the big changes are:

breaking changes

new functions

cc @mick @willwhite @miccolis

willwhite commented 8 years ago

Let's merge this. I'm fully onboard with the goal of maintaining less code and documentation (especially because the dyno docs to date have been... lacking). Looking back at the list of things dyno is really bringing to the table and it seems like the biggest ones are streaming support, multi-region support, and cli.

dyno navigates the differences between the aws-sdk's regular client and document client for you behind the scenes.

What does this mean? Just that you don't need to decide which client to use it advance and that dyno picks which client to use based on what calls you make?

with the exception of explicit stream functions, none of the function calls fan out into multiple HTTP requests

:clap:

As for when to bite off the upgrades for key downstream projects, we can play that by ear. I don't think it will be that bad if we split up the work.

rclark commented 8 years ago

By "navigates the clients" I mean dyno offers you one set of commands instead of the two that aws-sdk sometimes provides. For example:

willwhite commented 8 years ago

Gotcha, makes sense. Anything else before mergetown?

rclark commented 8 years ago

Yeah, the CLI needs to be updated/tested and there are some significant gaps in test coverage in the streaming code.

Also, while I agree that it is nice to know that dyno won't fan out your batch requests into multiple HTTP requests, I see us writing the same code in several places to take a list of things to get or things to write, and having to break that up into requestable chunks.

To solve this, how about a dyno function like:

dyno.batchWriteRequests = function(params) { ... };

... where params is identical to batchWriteItem parameters, but without limits on size/count. The function would return an array of AWS.Request objects that the caller could .send() in whatever concurrency settings they choose.

willwhite commented 8 years ago

@rclark adjustments look good to me. I'm on the fence about if a beta release is needed. If we do a 1.0.0 we can always do a 1.2.0 :)

mick commented 8 years ago

@rclark yeah should have released those were generated. There are some other formatting changes that happened when I ran npm run docs maybe changed in documentionjs? Or am I missing some other doc generation config?

otherwise LGTM

rclark commented 8 years ago

@mick note that package.json uses documentationjs@master. Maybe reinstall it and try the npm script again?

mick commented 8 years ago

@rclark hmm I just installed it for the first time. Maybe it changed since that last time it was run?

mick commented 8 years ago

I dont think it's an issue. I just wanted to flag for you since I wasnt familiar with that step in case I missed something.

rclark commented 8 years ago

No worries @mick. Leave it as is and we'll be in better shape once there's a new documentationjs tag.