Add type definitions

[demurgos]

This also updates the devDependency rx@4 to rxjs@5 because it bundles its own types. This is a semver minor update (feature).

Two comments regarding these types:

• They guarantee that if an error occurs, the first parameter of the callback will be exactly null. This is undocumented and untested but true according to the code. It would be better to update the doc and tests to reflect this since it's already implemented this way.
• I added a comment at the top of the types regarding two caveats. Async tasks using the done callback to return multiple values are rejected by the type checker, and the interface of callbacks allows unsound callback functions: the result can be assumed to always exist, despite the possibility of error. This matches Node types. I'd prefer to have stricter interfaces but ended up using Node's style for usability. I am not yet sure if it's the right thing to do.

Closes gulpjs/async-done#41

/cc @BurtHarris @phated

[demurgos]

Thanks for checking. I usually prefer to be more conservative when I have a doubt (e.g. Stream or ReadableStream) so having someone more familiar with the lib and the values actually expected is good.

Before Typescript 2 and the existence of @types on npm, I wrote definitions for a tool called typings. This is now in maintenance but maybe some of the members are available:

@unional @blakeembrey Do you have some time to review this PR? It is pretty short and adds types for async-done. I have few questions where you may help me:

• Is there a way to unify my VoidCallback and Callback<T> types without https://github.com/Microsoft/TypeScript/issues/5453? I'd like to have VoidCallback = Callback<void> but this is not equivalent in the case when my async task just wants to signal completion without returning any value:

  function task1(done: VoidCallback): void {
  done(null);
  }
  
  function task1(done: Callback<void>): void {
  done(null, undefined); // Here, the compiler forces me to pass `undefined` as a second parameter even if I don't care about it.
  }

  Edit: I still need to think about it but it may be possible in the specific case of async-done. I'm still interested if your have some remarks.

• What is your current strategy to handle dependencies modifying the environment? (@types/node, but also others such as @types/mocha). With typings we marked them as external ambient declarations and the end-user had to install them manually. The closest to this behavior with npm is to have them as devDependencies: there were some issues with major updates of @types/node at the beginning of @types because it caused duplicated conflicting global definitions caused by having @types/node as normal dependencies. Since then, I always used devDependencies for @types/node and kept my dependencies up-to-date and did not have any issues. Still, I see that Definitely Typed uses a normal dependency for @types/node. Is it because of their mono-repo structure so they can push updates of @types/node to all their packages immediately or was there an update in TS that mitigates the impact of incompatible global definitions in the dependency tree?

• Is there a trick to prevent (): void => {} from matching (done: VoidCallback): void => {}. If the function does not return any of the the async primitives, I want to force it to accept a callback. I cannot enforce here that the task actually calls it but if I make sure that it is not forgotten as an argument further checks for unusedVariables can pick this up.

[unional]

Is there a way to unify my VoidCallback and Callback types without Microsoft/TypeScript#5453?

It is also related to this: https://github.com/Microsoft/TypeScript/issues/12400 It is marked as committed for 2.8

What is your current strategy to handle dependencies modifying the environment?

Not sure what you mean, global augmentation?

Is there a trick to prevent (): void => {} from matching (done: VoidCallback): void => {}.

I assume its used in a callback (can't find that in your PR from skimming)? AFAIK, there is no way to prevent that.

🌷

[demurgos]

@unional

Is there a way to unify my VoidCallback and Callback types without Microsoft/TypeScript#5453?

It is also related to this: Microsoft/TypeScript#12400 It is marked as committed for 2.8

Great! I'm still waiting for variadic types but the issue you linked should help with this case. Glad to see it got scheduled.

What is your current strategy to handle dependencies modifying the environment?

Not sure what you mean, global augmentation?

Sorry I was still calling them "ambient declarations", but yes: this is what I had in mind. I am mentioning variable declarations that let you use variables without having to import them first. Example in @types/node and @types/mocha. Support for these declarations was really the strong point of typings and I am still not sure what's the best way to deal with them today.

I assume its used in a callback (can't find that in your PR from skimming)? AFAIK, there is no way to prevent that.

Yes, it was used in callbacks. The current tests only check that the file compile so I cannot test for expected compilation errors. This case is mentioned in a comment: here is the relevant part.

// I don't think TS is currently able to prevent the current code from compiling
// (`neverDone` matches with `(done: VoidCallback) => void` for example)
// asyncDone(neverDone, function(err: Error | null, result?: number): void {
//  console.log("Done");
// });

Passing neverDone fails at runtime, I don't think it's possible to catch this with type-checking.

I don't think it is supported currently, but I had an idea like this:

interface AsyncTask {
  (done: VoidCallback): void;
  length: 1;
}

This would add the constraint that functions have to have exactly one parameter. But I think that you are right: there are too many edge cases such as arguments or (...args) (or some crazier things like proxies) to make it works. That's why I said at the beginning that it's hard to get types right when callbacks are involved.

[demurgos]

Most of the comments were addressed: would it be possible to merge and publish it (semver feature)?

Regarding my questions in this thread: it's safer to keep global definitions (Node) as devDependencies and the issue to unify VoidCallback and Callback was postponed so it's better to not wait for it.

[phated]

@demurgos I've reviewed everything and it seems to make sense with my minimal knowledge (thanks for all the docs). I'll get this published.

[phated]

@demurgos whoops, the index.d.ts wasn't added to "files" in the package.json so I publishing as 1.3.1