AGBrown/pouchdb.d.ts

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> { }
    }
  1. thenable.PouchDB is a bad name (a real thenable does not have a catch anyway)
  2. thenable.PouchDB is just there to be the return type for the promise-based constructors (i.e. to prevent then showing up on your pouchdb db variable)
  3. promise.PouchDB is exactly an api.db.Promisable
  4. but pouchdb.promise.PouchDB is there because, ideally, you would be able to do var db : pouchdb.PouchDB. However we are splitting callable and promise interfaces to help end-users, so var db : pouchdb.promise.PouchDB is seemed marginally more explanatory (and has one less level of nesting) than var db : pouchdb.api.db.Promisable

@fredgalvao commented, rightly, that this structure is confusing. So ... how can we improve it?

Suggestions:

  1. Rename module promise to module promisable in keeping with #22.
  2. or rename module thenable to module promisable and remove module promise.
  3. Or just move the interfaces out of module db and use those instead of module callback and module 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 set db using the then 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> so new 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...)