search

steadyequipment / node-firestore-backup

Google Firebase Firestore backup tool
190 stars 51 forks source link

removed fluff and made asynchronous #17

Closed balupton closed 6 years ago

balupton commented 6 years ago

close #16

yoiang commented 6 years ago

Hey @balupton ! Your enthusiasm is appreciated! However this PR does not address the ask for specifying a maximum number of requests (making it async is the easy part! 😉).

A compromise that would be appropriate would be a well architected option to run either in serial or in parallel. Additionally for a PR to be accepted it will have to be written in ES6, with Flowtype notation and pass the commit tests.

balupton commented 6 years ago

No worries.

For configurable concurrency it will be just adding the concurrency property to the promise lib that I selected, and adding an option for it - however it seems unnecessary to make it configurable, as unlimited concurrency seems to work fine, which is the goal of parallel execution.

For the rest, it is what I considered fluff. So I won't amend the PR to re-add it, and will just continue on with my fork. Would you still like the copyright of the fork, or should I amend it to myself? Up to you.

FWIW at least to me, the modified code seems to improve the error handling, as it returns rejected promises rather than errors. From memory, returning an error just passes the error to the next .then handler - only returning the error within a rejected promise bubbles it up to the next .catch handler.

yoiang commented 6 years ago

Haha cheers!