types/node/index.d.ts

to authors (@DefinitelyTyped/DefinitelyTyped @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @Tyriar @DeividasBakanas @kjin @alvis Microsoft TypeScript (account can't be detected)). Could you review this PR?
👍 or 👎?

Open question - I've fixed up definitions for errbacks in the tests to be (error: Error|null, result: Result). In theory, this is what Node errbacks do by convention. However, I don't think anybody would like to trawl through the node codebase and make sure that nobody ever ever sends an undefined in case of an error.

Hence, should the definitions of Node builtin modules be changed to pass (error: Error | null | undefined) in general? This question is probably (hopefully :) ) too broad for this PR, but it is something we might have to think about if it's accepted (as it might encourage user code like if (err === null) { }, which I wouldn't trust for reasons above).

@akdor1154 Please fix the failures indicated in the Travis CI log.

@akdor1154 Please fix the merge conflict.
As for null vs null | undefined, I would wait until you actually see undefined coming out of node, assuming that their documentation guarantees that null should be sent instead.

@Andy-MS Documentation makes no such guarantee, convention states that null should be preferred.
Documentation varies module to module (and is sometimes entirely absent..), but for example fs states

The asynchronous form always takes a completion callback as its last argument. The arguments passed to the completion callback depend on the method, but the first argument is always reserved for an exception. If the operation was completed successfully, then the first argument will be null or undefined.

Examples generally always show error checks as a check for truthiness, if (err) { ... }.

If this were my own project the way I would type this is add a

type NodeErrBack<T, E=Error> = (error?: null | Error, result: T) => void

and use it throughout node/index.d.ts. Would that approach be acceptable?

Sure, although I would write error: Error | null | undefined as I don't think an optional parameter can come first.

should I add this to this PR? (it'll be a pretty big change, touching any function in node that takes a callback...)
EDIT also, are default generics allows in this?

It can be in a separate PR.
The node types haven't updated to ts2.3 yet so they don't support default generics. We could increment the version but it would need a good reason.

k, shall rebase+squash then continue discussion in a new PR.

The failing lint in duplexer3 is a result of this pr. The reason is because the cb parameter in stream._write is now typed as => void instead of => any. The failing statement in duplexer3 is

writable._write = (input, encoding, done) => {
  if (something) {
    return done();
  }
  ..
}

which is a common pattern, but fails no-void-expression in tslint if done() => void. Does this suggest callbacks should be typed as => any and not => void?

Besides the first argument in callbacks also the second one is optional - if the first one is not null.
Would be nice if this known dependency could be modeled somehow otherwise users have to use two checks or the ! operator.
Maybe the default type should be NodeJS.ErrnoException, I have seen several complains that checking the optional errno/code properties is not allowed by typescript.
I fear stepping to typescript 2.3 could be cumbersome as it would imply that all the > 1000 dependent modules have to use also >=2.3 to my understanding.

Regarding the issue with duplexer3: I would vote against use of any in callbacks just to make lint happy. Lint should help us not we should help lint.
I think we should try to avoid any where possible.

You don't have to change the type, you can just done(); return;.

@Andy-MS my point is more that return done() is idiomatic, and I don't want to create a situation where valid and idiomatic code causes a lint error. I think I would tend towards Flarna's approach that this is more a lint configuration problem though.

It won't cause a lint error for people not using DT's lint configuration. So I agree with @Flarna to use the most accurate type possible, and users who don't want a lint error on return done(); should not use that lint rule.

@akdor1154 Please fix the failures indicated in the Travis CI log.

@akdor1154 could you fix the test failures?

I'm closing this PR due to lack of activity. If you would still like to get in these changes, please open a new PR. Thank you!