Add type definitions #47

demurgos · 2018-01-23T16:17:28Z

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 #41

/cc @BurtHarris @phated

Updates the devDependency `rx@4` to `rxjs@5`. Closes gulpjs#41

phated

@demurgos I don't know much about these typings but I sent some questions/thoughts. They seem good. Do you know of anyone else you can ping that can review these at a higher level than me?

phated · 2018-01-23T19:27:04Z

index.d.ts

+ */
+import { ChildProcess } from "child_process";
+import { EventEmitter } from "events";
+import { Readable as ReadableStream } from "stream";


This can be Readable, Writeable or Transform/Duplex because we exhaust the stream.

I think it might be able to reference the base "Stream" instead? Not sure if it contains the indicators we use to check for stream.

I originally had Stream but wasn't sure if Writable streams were supported. Thanks for confirming that any Node stream is fine. I'll update it.

phated · 2018-01-23T19:32:20Z

test/observables.js


 function success() {
  return Observable.empty();
 }

 function successValue() {
-  return Observable.return(42);
+  return Observable.of(42);


Can you leave a comment that this was previously Observable.return in case someone was using the docs as a reference for older Rx?

Or maybe we should keep both dependencies (rx & rxjs) since 4.x is still published as rx??

rx is only used for the tests so I don't think we need to keep both versions as dependencies but it's definitely valuable to add a comment and mention it in the documentation. I'll do it.

phated · 2018-01-23T19:36:10Z

test/types/streams.ts

+import { Readable, Stream } from "stream";
+
+function readableSuccess(): Readable {
+  return undefined as any;


why are these returning undefined as any?

These file are compiled but never run. I just needed to check that a function returning a Readable is accepted.

I did not care about the implementation of the task (runtime behavior is tested in test/streams.js). Using any allows to bypass type-checking locally ("trust me, this function returns a Readable").
I wrote the body of the functions for other kinds of objects because they are one-liners and it is more helpful to visualize a concrete example. In the case of streams, the boiler plate was slightly higher so I just used a dummy body.
I'll replace it with return new Stream(); instead of replicating what's in the unit-tests.

demurgos · 2018-01-23T22:36:34Z

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 Proposal: Variadic Kinds -- Give specific types to variadic functions microsoft/TypeScript#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 · 2018-01-23T23:22:37Z

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

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 · 2018-01-24T00:09:23Z

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

- Document support for RxJS 4 and 5 - Accept `Stream` as a return value instead of `Readable` - Remove dummy body in stream tests. - Reword some comments. Closes gulpjs#41

demurgos · 2018-05-08T09:19:02Z

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 · 2018-05-13T20:35:07Z

@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 · 2018-05-13T21:06:04Z

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

Add type definitions

78ea929

Updates the devDependency `rx@4` to `rxjs@5`. Closes gulpjs#41

phated reviewed Jan 23, 2018

View reviewed changes

demurgos force-pushed the issue-41 branch from 0aec7a4 to 51de4ea Compare January 24, 2018 00:18

Minor fixes to type definitions

592b94e

- Document support for RxJS 4 and 5 - Accept `Stream` as a return value instead of `Readable` - Remove dummy body in stream tests. - Reword some comments. Closes gulpjs#41

demurgos force-pushed the issue-41 branch from 51de4ea to 592b94e Compare May 8, 2018 09:13

demurgos mentioned this pull request May 8, 2018

Add type definitions gulpjs/glob-watcher#38

Open

phated merged commit d79bddf into gulpjs:master May 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type definitions #47

Add type definitions #47

demurgos commented Jan 23, 2018 •

edited

Loading

phated left a comment

phated Jan 23, 2018

phated Jan 23, 2018

demurgos Jan 23, 2018

phated Jan 23, 2018

phated Jan 23, 2018

demurgos Jan 23, 2018

phated Jan 23, 2018

demurgos Jan 23, 2018

demurgos commented Jan 23, 2018 •

edited

Loading

unional commented Jan 23, 2018

demurgos commented Jan 24, 2018 •

edited

Loading

demurgos commented May 8, 2018 •

edited

Loading

phated commented May 13, 2018

phated commented May 13, 2018

Add type definitions #47

Add type definitions #47

Conversation

demurgos commented Jan 23, 2018 • edited Loading

phated left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demurgos commented Jan 23, 2018 • edited Loading

unional commented Jan 23, 2018

demurgos commented Jan 24, 2018 • edited Loading

demurgos commented May 8, 2018 • edited Loading

phated commented May 13, 2018

phated commented May 13, 2018

demurgos commented Jan 23, 2018 •

edited

Loading

demurgos commented Jan 23, 2018 •

edited

Loading

demurgos commented Jan 24, 2018 •

edited

Loading

demurgos commented May 8, 2018 •

edited

Loading