Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scan and query always return AsyncIterators #17

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hayd
Copy link
Contributor

@hayd hayd commented Mar 3, 2020

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
Copy link
Owner

@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
Copy link
Contributor Author

hayd commented Mar 3, 2020

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
Copy link
Owner

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

@hayd
Copy link
Contributor Author

hayd commented Mar 3, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to use Promise<Doc | AsyncIterableIterator<Doc>> ?
2 participants