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

Add getMany: get multiple documents by id #10

Merged
merged 11 commits into from
Nov 5, 2019

Conversation

thomastoye
Copy link
Contributor

Didn't find any test instructions, some help to finish this would be appreciated :)

@kossnocorp
Copy link
Owner

Wow, I work with Firestore since the day it was released and I didn't know this ability exists! Thank you so much for the PR! Sorry for nonexisting docs about contributing and testing. Let me fix that and write you an instruction on how you can run tests.

@kossnocorp
Copy link
Owner

Here we go: https://github.com/kossnocorp/typesaurus/blob/master/CONTRIBUTING.md

Please tell me if you have any questions regarding it.

Copy link
Owner

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the undefined problem.

src/getMany/index.ts Outdated Show resolved Hide resolved
src/getMany/index.ts Outdated Show resolved Hide resolved
@thomastoye
Copy link
Contributor Author

What do you think about the onMissing strategy like this?

async function getMany<Model>(
  collection: Collection<Model>,
  ids: readonly string[],
  onMissing: (id: string) => Model = (id) => { throw new Error(`Missing document with id ${id}`) }
): Promise<Doc<Model>[]> {
  ...

@thomastoye
Copy link
Contributor Author

Hi @kossnocorp, I added tests and fixed the undefined problem.

@kossnocorp
Copy link
Owner

What do you think about the onMissing strategy like this?

I love it, it totally makes sense!

Copy link
Owner

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please accept my fixes: thomastoye#1

Add few fixes to the getMany PR
@kossnocorp kossnocorp merged commit 03f5a00 into kossnocorp:master Nov 5, 2019
@kossnocorp
Copy link
Owner

Thank you a lot for the PR! I'll ship it with a new version.

@kossnocorp
Copy link
Owner

I've shipped getMany with v2.1.0

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.

2 participants