Simplify PouchDB ctor definitions
Opened this issue · 3 comments
Relates to #22
Consider:
/** The api module for the pouchdb callback pattern */
module callback {
/** The main pouchDB interface (callback pattern) */
interface PouchDB extends api.db.Callback { }
}
/** The api module for the pouchdb promise pattern */
module promise {
/** The main pouchDB interface (promise pattern) */
interface PouchDB extends api.db.Promisable { }
}
/** The api module for the pouchdb promise pattern (constructor only) */
module thenable {
/**
* Special case class returned by the constructors of the promise api pouchDB.
* Usually only a `pouchdb.promise.PouchDB` reference would be kept, assigned by
* the `then` of the constructor.
*/
interface PouchDB extends promise.PouchDB, async.Thenable<promise.PouchDB> { }
}
thenable.PouchDB
is a bad name (a realthenable
does not have acatch
anyway)thenable.PouchDB
is just there to be the return type for the promise-based constructors (i.e. to preventthen
showing up on your pouchdbdb
variable)promise.PouchDB
is exactly anapi.db.Promisable
- but
pouchdb.promise.PouchDB
is there because, ideally, you would be able to dovar db : pouchdb.PouchDB
. However we are splitting callable and promise interfaces to help end-users, sovar db : pouchdb.promise.PouchDB
is seemed marginally more explanatory (and has one less level of nesting) thanvar db : pouchdb.api.db.Promisable
@fredgalvao commented, rightly, that this structure is confusing. So ... how can we improve it?
Suggestions:
- Rename
module promise
tomodule promisable
in keeping with #22. - or rename
module thenable
tomodule promisable
and removemodule promise
. - Or just move the interfaces out of
module db
and use those instead ofmodule callback
andmodule promise
I personally don't like this (which would be a vote against option 2):
var dbp: pouchdb.api.db.Promisable;
new PouchDB("name")
.then(db => dbp = db);
compared to
var dbp: pouchdb.promise.PouchDB;
new PouchDB("name")
.then(db => dbp = db);
Purely because dbp
is a PouchDB
, not a Promisable
Option 3 might be the best option. module db
is a little superfluous - I only put it there to be clear where the main db interfaces were defined, but then ended up needing the callback
and promise
modules which are only there to improve the external naming.
Update from IRC chat with daleharvey:
TL;DR: pouchdb officially does not support the ctor returning a promise, so we should remove it. Updating this issue accordingly (cc @fredgalvao).
[14:10:54] <AndyBrown> Did I read somewhere that creating a pouchdb is asynchronous, therefore
var db = new PouchDB()
is wrong, and the preferred method is to set db through the completion of a promise? Or did I just make that up?
[14:11:25] <daleharvey> its confusing,var db = new PouchDB()
is the recommended way to setup a pouchdb instance
[14:12:12] <AndyBrown> thanks. So we shouldn't setdb
using thethen
of a promise?
[14:13:19] <daleharvey> nope, thats the old way that we have moved away from
[14:12:26] <daleharvey> we do need an asyncronous setup, but we do it behind the scenes
[14:12:48] <daleharvey> sonew PouchDB().info()
will always work fine
[14:13:42] <AndyBrown> moved away as in pre v3.4, or moved away as in v4.0?
[14:14:02] <daleharvey> yeh for a long time its not been recommended
[14:14:23] <daleharvey> still in there for backwards compat, but it has problems
[14:16:11] <AndyBrown> ... so with reference to #23: if I understood you properly, in fact we don't need to (or even, should absolutely not) have the object returned from the ctor extend promise?
[14:16:53] <daleharvey> nope you definitely shouldn't, it causes all sorts of problems
[14:17:39] <daleharvey> I mean, the exact problems you are describing there are what we were having issues with
[14:17:58] <daleharvey> pouchdb is a promise to itself, just strange
...
[15:10:43] <AndyBrown> daleharvey: if I remove promise from the object shape returned by the ctor, then I won't be able to do tests like: test.basics.ts#L41-L45. Is it safe to just comment those out? If not I can probably find a workaround just for that test file (i.e. optionally add promise to the returned shape to support legacy use cases)
[15:11:13] <AndyBrown> permalink to that code: a6b2b045/test.basics.ts#L41-L45
[15:11:22] <daleharvey> I would just take any tests / usage of the promise return out of there
Updated this issue, and the code (7ea6e81) following feedback from daleharvey. Comments from anyone? (cc @fredgalvao).
That's awesome news! I didn't like that thenable|promise
thing. I'm wondering why such things aren't removed from the code base on major version bumps though, as in 4.0.0 (which removed bluebird 😢 ).
Doesn't that imply that the callback parameter passed on to the constructor is also deprecated and should not be advised? (In the end it's just another way to deal with async behaviour on the db instance, so it should inherit the same reasoning...)