chiefbiiko / dynamodb

deno <3 dynamodb
MIT License
25 stars 11 forks source link

scan and query always return AsyncIterators #17

Open hayd opened 4 years ago

hayd commented 4 years ago

Fixes #4

@chiefbiiko This also makes the DynamoDBClient interface more explicit... This would be a breaking change, but IMO it's a more user-friendly API.


If you wanted convenience functions for scan+query that return Promise we can special case i.e.

  dyno.queryOnce = async (params: Doc, options?: Doc): Promise<Doc> => {
   // TODO handle options === undefined
    options.iteratePages = false;
    for await (page of dyno.query(params, options)) {
      return page
    }
  }

  dyno.scanOnce = async (params: Doc, options?: Doc): Promise<Doc> => {
    options.iteratePages = false;
    for await (page of dyno.scan(params, options)) {
      return page
    }
  }

(save for a future PR - naming is discussion worthy).

chiefbiiko commented 4 years ago

@hayd thanks for ur input, let me try to paper my thoughts: i want dynamodb to reflect the aws api amap - ideally no additional brain-drain layer

my concern regarding always-ait is that counting like

const { Count } = await dyno.scan({ TableName: "Tisch", Select: "COUNT" });

wouldnt work - would have to be

const { Count } = await dyno.scan({ TableName: "Tisch", Select: "COUNT" }, { iteratePages: false });

too unintuitive solution could be to point users to dyno.scanOnce or intro dyno.count but then thats dominos and bloat

also the scan op is expensive and sth to avoid by db design i dont necessarily want to "optimize" this part of the client with custom logic

but need some more thought on this..

hayd commented 4 years ago

also the scan op is expensive and sth to avoid by db design

Whilst it is expensive it's often the case you need more that one page worth. 🤷‍♂

It would be:

const { Count } = await dyno.scan({ TableName: "Tisch", Select: "COUNT" }, { iteratePages: false });
// wouldn't work either, it would be:
const { Count } = await dyno.scanOnce({ TableName: "Tisch", Select: "COUNT" })

I don't think it's dominoes as these are the only two ops that have pagification.

You could also do it the other way scanMany or scanIter (or something). At the moment using it as asyncIterable is the more painful thing to use, so if you think scanIter is used less often maybe it makes sense to have it being the different name.

chiefbiiko commented 4 years ago

leanin towards preserving scan but exposing scanIter to get an ait with guarantee

hayd commented 4 years ago

Is a trick to define a new class that is both awaitable and async iterable?

i.e. await scan() returns :Doc and for await (const page:Doc of scan()) {} also works.