Reject promises when an error occurs

[nolanlawson]

Sorry for creating so many issues. Hope you understand it's because I really, really like this plugin. :)

Stuff like this is kind of a bad practice when it comes to promises. When an error occurs, you really ought to surface that to the user. A better pattern would be:

new Promise(function (resolve, reject) {
  // do some async stuff
  .catch(reject);
})

or more simply:

function myFuncThatReturnsAPromise() {
  return doSomeStuff(); // implicitly rejects from the client's perspective if there's an error anywhere
}

[redgeoff]

No problem on the issues, please keep them coming :)

I'm still relatively inexperienced w/ promises. Let's take two examples. Here's what I would propose:

exports.all = function () {
  var db = this;
  return new Promise(function (fulfill, reject) {
    var docs = {},
        deletions = {};
    db.allDocs({include_docs: true}, function (err, doc) {

      if (!doc || doc.rows.length === 0) {
        fulfill();
        return;
      }

      // sort by createdAt as cannot guarantee that order preserved by pouch/couch
      doc.rows.sort(function (a, b) {
        return a.doc.$createdAt > b.doc.$createdAt;
      });

      doc.rows.forEach(function (el, i) {
        if (!el.doc.$id) { // first delta for doc?
          el.doc.$id = el.doc._id;
        }
        if (el.doc.$deleted) { // deleted?
          delete(docs[el.doc.$id]);
          deletions[el.doc.$id] = true;
        } else if (!deletions[el.doc.$id]) { // update before any deletion?
          if (docs[el.doc.$id]) { // exists?
            docs[el.doc.$id] = exports.merge(docs[el.doc.$id], el.doc);
          } else {
            docs[el.doc.$id] = el.doc;
          }
        }
        if (i === doc.rows.length - 1) { // last element?
          fulfill(docs);
        }
      });
    }).catch(reject);
  });
};

keeping in mind that I'd only like all() to fulfill after all docs have been loaded

and

function getAndRemove(db, id) {
  return new Promise(function (fulfill, reject) {
    db.get(id).then(function (object) {
      db.remove(object).then(fulfill).catch(reject);
    }).catch(function () { // not found?
      fulfill();
    });
  });
}

Are these better? Can you think of a better way to refactor these?

[nolanlawson]

Sent some PRs your way. :)

[redgeoff]

Hey @nolanlawson, I just merged https://github.com/redgeoff/delta-pouch/pull/11, which should greatly improve the way promises are handled throughout delta pouch. I know it is ridiculously large amount of changes for one commit, but I figure this project still hasn't gotten past an initial release so I'd just bite the bullet and fix it all at once. In the future, I'll break this stuff up.

When you get a chance, could you please take a look at the new code and see if there are any more promises that should be refactored? There are two areas that I'm still not to sure about:

https://github.com/redgeoff/delta-pouch/blob/master/index.js#L47

and

https://github.com/redgeoff/delta-pouch/blob/master/test/test.js#L262-L352 - Is this the best way to test these cases?

[nolanlawson]

Yep, no prob. Moving fast this early on is totally okay, and anyway, it's your project. :)

[nolanlawson]

Sent in a PR for the first one. For the second one, I believe your tests are ending early, because the promise is resolving before the onCreate/onUpdate/onDelete even gets invoked.

In any case, you're right that you should use event emitters instead. PouchDB already emits a 'change' object, so you could just piggyback on that and emit your own events. It may get confusing, though, because we also emit some 'created' and 'deleted' events, I believe. (It's not documented and we should really go through it with a fine-toothed comb.)

[nolanlawson]

There is also a Promise.all function you may be interested in to replace your each function.

[nolanlawson]

More about promises: https://www.promisejs.org/patterns/

[redgeoff]

Yeah, I first tried using Promise.all in cleanup(), but the problem I then had was that each cleanupDocs call has to occur in sequence and Promise.all doesn't guarantee that the promises will be executed in sequential order.

[nolanlawson]

You can do promises in sequence. There is a secret pattern:

var chain = Promise.resolve();

var tasks = ...; // this is an array of promises
for (var i = 0; i < tasks.length; i++) {
  chain = chain.then(tasks[i]);
}
chain.then(...); // by the time then() is called, all of them have been executed in sequence

[nolanlawson]

Er, sorry, I mean that should be an array of promise factories, i.e. functions that each return a promise. It's hard.

[redgeoff]

Thanks for the info on the sequential chaining. I've fixed that with https://github.com/redgeoff/delta-pouch/pull/16.

As for emitting events: I'm thinking about prefixing the events with delta- so that they are kept separate from pouch core, e.g. delta-create, delta-update, and delta-delete. What do you think?

[redgeoff]

Also, I'd have to create some mechanism for setting the getItem (https://github.com/redgeoff/delta-pouch/blob/master/index.js#L74) function. I'm thinking something like one of the following. Anything look pouch-like?

db.register('get-item', getItem);
db.getter('item', getItem);
db.registerGetItem(getItem);
db.deltaConfig({ item: getItem });

[nolanlawson]

If I were you, I would actually not concentrate on events for now. Yes, you are right to prefix, but in my experience event emitters are just really hard to grok (I've screwed it up many times), and you may want to nail down the core API first. Just a suggestion.

[redgeoff]

https://github.com/redgeoff/delta-pouch/pull/18 contains the new event emitter code. I'm sure it could use some more fine tuning, but I'm pretty happy with how it is working. I rolled my own events as I didn't have much luck piggybacking off of pouch's existing events and I figured it would probably be better if they were kept separate anyway.