Make definitions work nicely with third-party promise implementations

[fredgalvao]

This definition file/project (pouchdb.d.ts) should not create it's own definition of a Promise/Thenable, (edit currently defined at these lines) even if said definition is A+ and/or spec compliant, because it makes extending the api invasive if anyone does global.Promise = require('bluebird'); as suggested by the corresponding blog post.

We should expect definitions for it instead, like this one, so that any other definition file that happens to override the standard (like this other one) can be used by editors and IDEs correctly.

I'll try and work on a PR for this, but can't guarantee it.

[AGBrown]

Agreed. This has been on my todo list from the start. Initially I just had to
get something in there, but it should definitely be changed. PRs welcome, but if
you don't have time maybe one of us will.

[fredgalvao]

Ok, so:
I managed to make it work in two different ways, and I can't make the decision on which one to use by myself here, so I need some input @AGBrown .

I'm leaning towards the second one, FYI.

Futuristic approach

If I change the file tsconfig.json(created by me) to target typescript compilation to es6, I can point directly to the official Promise interface/class implementation from lib.d.ts, and no other file is needed on the project. This implies that the interface defined by this .d.ts of ours is supposed to be used on es6 ready environments, which is not entirely true. It doesn't force anyone to actually be on es6 though, it's just a tecnical assumption, it will work anywhere anyway.

Dependency approach

We, and the users of our .d.ts, will need to import at least 1 file that defines a Promise interface/class. In our case (pouchdb.d.ts) we can/should import es6-promise.d.ts from DefinitelyTyped into the project, and users should be warned to do the same (be it es6-promise, bluebird, lie, or changing their own tsconfig.json to es6 output).

[AGBrown]

I think it might be good to get some input on this. My gut feel would be to follow the conventions (if any) in the DT repo.

edit: brief review of DT repo d.ts files:

• acl.d.ts: references bluebird.d.ts and uses import Promise = require("bluebird");
• alt.d.ts: references es6-promise.d.ts
• amqplib.d.ts: references when.d.ts
• angular-protractor.d.ts: references selenium and extends it
• atom.d.ts: references q.d.ts
• selenium-webdriver.d.ts: defines it's own IThenable<T>
• chai-as-promised.d.ts: referencespromises-a-plus.d.ts
• highland.d.ts: same state as us (defines their own, todo to fix)
• less.d.ts: same state as us (carbon copies definition from es6-promise.d.ts)

I got as far as "d" for dexie, and also did highland and less and then stopped: there are 180 files with "Promise" in them.

Also

I will ask in freenode's #typescript channel and see if I get any feedback. edit: I would have thought the second one was more appropriate; I hadn't even thought of the first one.

Edit: feedback from #typescript channel:

[12:06:49] <enlight> AndyBrown: I don't think there are any conventions, I've seen some pull in Bluebird typings, others Q, etc. Personally I think the Promise typings should be provided by the user.

re: the two suggested alternatives

[13:18:54] <enlight> AndyBrown: Honestly I don't see the difference, either way you assume the type definition are provided by someone else.
[13:58:31] <AndyBrown> enlight: If i follow what other d.ts files do (e.g. https://github.com/borisyankov/DefinitelyTyped/blob/6f6e5c7dd/acl/acl.d.ts#L6). Does that mean the user can easily replace it with their chosen promise definition later?
[14:33:37] <enlight> AndyBrown: That actually makes it more difficult since the paths and filenames would have to match. I find the triple-slash references in DT typings really annoying actually, they do it that way so that tsd can automatically download typings for dependencies. However, it's easy to end up with conflicts if for example your package pulls in node.d.ts via a triple-slash reference, and one of your package dependencies pulls in node.d.ts via a triple-slash reference in it's public d.ts, then you end up with duplicate node.d.ts files in your compilation context.
...
[14:38:48] <AndyBrown> If I don't use a /// to a default promise implementation, and I don't define my own Promise/Thenable inside the d.ts - how do i get the d.ts to compile and pass the tests for inclusion in DT - I'm sure I'm missing something obvious here
[14:47:29] <enlight> AndyBrown: Can't you just triple-slash reference es6-promise.d.ts in your test file?

[AGBrown]

One note I would make is that importing a promise definition means we lose the ability to provide the shape of the Error object for the onReject/reject function. Currently we are trying to define an error shape

    interface Error {
        error?: any;
        message?: string;
        status?: number;
    }

which is then used by the promise definition. If we use a standard promise library the user would need to cast their error as the Error object.

        /** A Promises/A+ `then` implementation */
        then<R>(onFulfilled?: (value: T) => Thenable<R>|R, onRejected?: (error: Error) => Thenable<R>|R): Thenable<R>;
        /** `catch` implementation as per the pouchdb example docs */
        catch<R>(onRejected: (error: Error) => Thenable<R>|R): Thenable<R>;

That note comes with the caveat that there is an open issue/discussion related to errors - so the error debate isn't fully settled yet.

Edit feedback on this from #typescript irc:

[12:36:25] <enlight> AndyBrown: I think it's easy enough for the user to put in the expected error type in catch(), so long as you provide the typings for the Error (even better if there are actual Error subclasses).
... since Promise.catch is declared as catch(onrejected?: (reason: any) => T | Promise<T>): Promise<T>, the user can simply write catch((error: couchdb.Error) => { ... }) if you've provided the type definition for Error.

Also - is it possible that we could export our promise definition instead and allow people to override it by referencing their chosen promise d.ts after the pouchdb.d.ts? I'm not sure if that would work? Thinks: possibly not as that would add methods to it, not replace it?

Thoughts?

[fredgalvao]

Nice digging there!

So:

• I personally don't like importing stuff atm as it implies a specific way to compile your project, which might not always be the case for end users. However, my experience with typescript and standard module definiton/import is not that great, and I could be wrong in this one.
• Yes, I've seen plenty of definition files on DefinitelyTyped referencing each other using the ///... way of life, and I like that because it works and is understood in pretty much every typescript coding/compiling environment I know so far (IDEA, tsc, VSCode). And yes, I know that users would have to edit the relative path to their own offering of the promise implementation file, I've done that for most of the definition files I use on my project tha references angular or jquery, and I don't think that's a big deal. We could, however, not reference any promise definition on pouchdb.d.ts and do it on the tests, and just warn users that it needs to be supplied by them somewhere else on their ts project.

Real stuff

I understand that either approach would expect the user to provide a Promise definition/implementation somehow, be it through another .d.ts or through es6 compilation. I just don't think there's that many peopĺe compiling to es6 at the moment, thus my bias towards the second approach.

Regarding the Error thing, I wish the spec would assume an Error object to be supplied on .catch, instead of a any, but meh. So, I don't think it's a good trade to increase complexity of our Promise stuff because of that, as users specifying their own parameters for the onRejetion cb is not that big of a deal IMHO.

The only way I see we could try and achieve both goals (use the global Promise definition)+(offer error typing) is to override the global Promise implementation with our own flavour of catch. Typescript declaration merging allows for that to be ok so that it would understand all mergings (bluebird methods, es6 methods, and our custom catch+then). However, I can already see that it would only offer typings to the methods we define, so custom methods like .finally(), would be defined by bluebird but wouldn't have our custom Error object, thus generating inconsistence or partial feature.

For the sake of simplicity, I'd rather leave the Error thing to the user to define (pointing to our Error) and be standard everywhere else.

Let me know what you think.

[AGBrown]

I personally don't like importing stuff

I was fast and loose with the word import, that's my fault. I meant: "One note I would make is that /// referencing a promise definition ..."

We could, however, not reference any promise definition on pouchdb.d.ts and do it on the tests, and just warn users that it needs to be supplied by them somewhere else on their ts project.

I am currently thinking this is the way to go. My one worry is that the IDE will constantly complain about the use of Promise as a return type inside the d.ts itself which would be annoying when developing the d.ts, but I think we can get round that with the tsconfig.json.

We can (should) also run tests for a number of promise libs (es6, bluebird, q, when) referenced by /// using travis to know if we'll have any issues with specific promise libs.

I understand that either approach would expect the user to provide a Promise definition/implementation somehow, be it through another .d.ts or through es6 compilation.

Yes - agreed, it would expect that. However the alternative, as enlight said, is to set a /// reference inside the d.ts file which may clash with whichever promise lib the user is using elsewhere; that seems worse to me. I think it's better to always require them to do one line of work to get it to work than for it to break unexpectedly some of the time without them knowing why. I would argue that's what the readme.md is for - for them to read instructions on how to use it.

thus my bias towards the second approach

I am definitely with you on this.

Regarding the Error thing, I wish the spec would assume an Error object to be supplied on .catch, instead of a any, but meh. So, I don't think it's a good trade to increase complexity of our Promise stuff because of that ... be standard everywhere else

Agree with all of that. As enlight said, they can just do catch((error: pouchdb.async.Error) => { ... }), or similar. It doesn't make it automatic, but I'm still not sure you'd get that error shape every time anyway. The method documentation can always be used to suggest when they should use the above form.

So where does that leave us?

I think it may be time to write some code, see if we both like it and then commit it. I'll see if I have time this weekend, if not and you get there first then even better. If we both do it and it is slightly different we can take the best of both.

[fredgalvao]

Oh, on the import thing, I wasn't refering to that comment of yours, I was giving my opinion on the examples you provided from DefinitelyTyped. 😉

So, I've already done that all work before we came to the conclusion, as I needed a working version to progress on another feature of my own project :).

I just need to switch the tsconfig.json compilation output to es6 and remove the reference'd definition I created. I also did some other small improvements to make pouchdb.d.ts work on typescript 1.6 (check the "Catching more errors" section here, details here), so I want to actually split these changes into different PR branches. I should have it ready in the next few days.

[AGBrown]

Sounds like a plan. Thanks again for the all your work on this.