Regression in .compact?

[tlvince]

In 3.2.1, given:

var db = new PouchDB('db');
db.compact()
  .then(function(response) {
    console.log(response);
  })
  .catch(function(error) {
    console.error(error);
  });

… logs undefined. Same if I use callbacks. In 3.2.0, this is returning the _local/compaction response as expected.

• 3.2.0 test case
• 3.2.1 test case

NB, also note I'm seeing a possible migration error if I run the 3.2.0 test case after 3.2.1:

// Promise
[Error] TypeError: undefined is not a function (evaluating 'tasks[i - 1](tx, nextMigration)')
    (anonymous function) (pouchdb.js, line 3949)

// Callback
[Error] {"status":500,"name":"Object","message":"the statement callback raised an exception or statement error callback did not return false"}
    (anonymous function) (d97da61674b027c83f161854bbbd97139efcb9a8, line 24)

[nolanlawson]

Yep, I forgot to mention in the blog post that there is a migration. I'll fix that.

As for compact, it's supposed to resolve with {ok: true} because that's what CouchDB does. Neither the 3.2.0 behavior nor the 3.2.1 behavior is correct. A pull request would be welcome. :)

[marten-de-vries]

So compact() has always resolved to undefined until 3.2.0. For 3.2.1 it was changed back (#3111).

Changing it to the CouchDB-like {ok: true} makes sense, but is technically a backward-incompatible api change (so 4.0)?

[tlvince]

Thanks for the heads up.

3.2.0 is resolving something like:

{
  id: '_local/compaction',
  ok: true,
  rev: '0-2'
}

… which makes more sense to me than a straight undefined, but agreed, mirroring what CouchDB does is best.

[nolanlawson]

@tlvince _local/compaction is an implementation detail; I think it was a bug that we ever exposed it.

@marten-de-vries AFAIK the docs have always been silent about what the promise is supposed to resolve with. So I don't see it as a breaking change; just a bug.

BTW I confirmed that {ok:true} is what CouchDB does:

$ curl -X POST http://127.0.0.1:5984/test/_compact -H 'Content-type:application/json' -d '{}'
{"ok":true}

[marten-de-vries]

@nolanlawson Alright, I see semver as a bit more strict (don't change return values of methods, period.) But on the other hand I can't see this breaking much except my Python-PouchDB tests, so I'm fine with {ok: true} instead. I'll open a PR for this, since I already changed this once for #3111 I know where to look. 😀

[nolanlawson]

Thanks, I would just prefer not to introduce a big version change for something that most people will not care about. 😛

Also my rule of thumb is that undocumented behavior is not something we need to adhere strictly to. :)

[marten-de-vries]

This has been fixed (2786090 & beaddad)

[davidpfahler]

Hi all, I'm not sure how to exactly articulate this, but this breaks existing code of mine completely. Some of you said that this would not be an actual regression but only in tests, so I wanted to let you know this isn't the case.

I have my pouch db set to ^3.1.0 in my package.json. I don't know what exact version it had when I last touched it and it still worked, but I can definitely tell you that when I upgraded to 3.2.1, this broke my entire database on second load (i.e. after it was compacted once). I had my PouchDB set to auto_compaction: true. When I disabled this setting, deleted all data and started the app again, the error went away. My exact error message was: "undefined is not a function (evaluating 'tasks[i - 1](tx, nextmigration)')" which is how I found this issue.

[nolanlawson]

@davidpfahler that nextmigration thing is migration code... was this in WebSQL or IndexedDB?

If you can provide a test case to reproduce, that would be great, although I understand that this kind of migration stuff is tricky. 😛