Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify PouchDB ctor definitions #23

Open
AGBrown opened this issue Sep 23, 2015 · 3 comments
Open

Simplify PouchDB ctor definitions #23

AGBrown opened this issue Sep 23, 2015 · 3 comments

Comments

@AGBrown
Copy link
Owner

AGBrown commented Sep 23, 2015

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 Rename Promise interfaces #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.

@AGBrown
Copy link
Owner Author

AGBrown commented Sep 30, 2015

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

@AGBrown AGBrown changed the title PouchDB ctor definitions naming Simplify PouchDB ctor definitions Sep 30, 2015
AGBrown added a commit that referenced this issue Sep 30, 2015
Removed following feedback from daleharvey - pouchdb
as a promise is only there to support legacy code.
@AGBrown
Copy link
Owner Author

AGBrown commented Sep 30, 2015

Updated this issue, and the code (7ea6e81) following feedback from daleharvey. Comments from anyone? (cc @fredgalvao).

@fredgalvao
Copy link
Contributor

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...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants