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

Return new documents on add, handle offline add #55

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattiaseyram
Copy link

@mattiaseyram mattiaseyram commented Oct 25, 2020

Thanks for the awesome library @nandorojo!

Related github issue :octocat:: #56

Changes

  • return added docs on successful useCollection add call
  • Synchronously set docs in useCollection add if offline

I noticed the add function provided by useCollection has Promise<void> as its return type. This could be improved by returning the added documents (including their new IDs) so that add can be used

I realize this could be an anti-pattern since we're unaware as to what actually happens on the server if we use a batch add. This could be improved by only returning the doc ids.

@nandorojo
Copy link
Owner

What if we made this behavior optional, with some sort of argument that lets you opt out of it? Something like silenceErrors? (Not sure about the name)

@cheunjm
Copy link

cheunjm commented Dec 26, 2020

Any updates on your discussion here? Just want to point out that it might be useful to also add an "add" document function under 'hooks/static-mutations' (after this pr is merged)

@nandorojo
Copy link
Owner

nandorojo commented Dec 26, 2020

Hey, there hasn't been much movement here. I commented here too.

For explanation on why there isn't currently a static add mutation, you can see my thoughts at #38 (comment). I've gone back and forth about it, the reason being that we can't update the SWR cache. So all we'd be doing is remaking the firestore add function. Maybe that's fine, not sure.

@nandorojo
Copy link
Owner

I thought more about this, and I think it's the right idea. I'll try to implement and test it when I have time in the next week.

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.

3 participants