@types/node: enable strictnullchecks, fix resulting issues #20542

akdor1154 · 2017-10-13T03:54:24Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes:
- module.parent (null -> undefined) - verified with repl on node 8.2.1. No mention of null in docs.
- assert.throws Error type optional - https://nodejs.org/api/assert.html#assert_assert_throws_block_error_message ("if specified")
- streams (_destroy callback)
  - https://nodejs.org/api/stream.html#stream_readable_destroy_err_callback
  - https://github.com/mcollina/node/blob/master/lib/internal/streams/destroy.js#L32 (on the Error parameter being optional_
- streams (simple initializer functions being equivalent to _internal method overrides) - https://nodejs.org/api/stream.html#stream_new_stream_readable_options
- readline (data is ignored if key is specified) - https://nodejs.org/api/readline.html#readline_rl_write_data_key
- inspector (properties are optional) - https://github.com/ChromeDevTools/devtools-protocol/blob/master/json/js_protocol.json (note className and exceptionDetails are both listed as optional)
Increase the version number in the header if appropriate.
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

Update @types/node to successfully work with strictNullChecks: true. Also var -> let in node-tests.ts. The first commit just fixes the "use before declaration" errors local to node-tests, the rest go on to fix the actual declaration problems detected as a result.

I am a bit confused that I was unable to find any existing discussion over this, sorry if I am unwittingly stomping all over existing drama.

-- Jarrad

dt-bot · 2017-10-13T03:54:26Z

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 👎?

akdor1154 · 2017-10-13T04:33:03Z

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

typescript-bot · 2017-10-13T05:56:52Z

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

Flarna · 2017-10-13T06:31:00Z

types/node/index.d.ts

@@ -5036,6 +5038,7 @@ declare module "stream" {
            wrap(oldStream: NodeJS.ReadableStream): Readable;
            push(chunk: any, encoding?: string): boolean;
            destroy(error?: Error): void;
+            _destroy(error: Error | null, cb: (e: Error | null) => any): void;


actually _destroy gets the error passed to destroy so I think here we have and exception to the Error | null rule.
And I think the return type of cb should be void.

destroy contains the call this._destroy(error || null) - https://github.com/mcollina/node/blob/master/lib/internal/streams/destroy.js#L32 .

Ah yes, you are right.

Flarna · 2017-10-13T06:32:03Z

types/node/index.d.ts

-            _destroy(err: Error, callback: Function): void;
+            _write(chunk: string | Buffer | any, encoding: string, callback: (e?: Error) => any): void;
+            _writev(chunks: Array<{ chunk: string | Buffer, encoding: string }>, callback: Function): void;
+            _destroy(error: Error | null, cb: (e: Error | null) => any): void;


As in Readable.

Flarna · 2017-10-13T06:46:48Z

types/node/index.d.ts

@@ -123,7 +123,7 @@ interface NodeModule {
    id: string;
    filename: string;
    loaded: boolean;
-    parent: NodeModule | null;
+    parent: NodeModule | undefined;


I can remember that parent may be also null in some cases but I can't remember the exact use case.
Please adapt also the class NodeJS.Module accordingly.
At least in source I found following new Module('internal/preload', null); so null is used to init parent.

ghost · 2017-10-16T20:44:57Z

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

akdor1154 · 2017-10-16T23:40:51Z

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

ghost · 2017-10-16T23:47:38Z

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

akdor1154 · 2017-10-16T23:48:39Z

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?

ghost · 2017-10-16T23:54:21Z

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.

akdor1154 · 2017-10-16T23:59:46Z

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

akdor1154 · 2017-10-17T02:53:54Z

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?

Flarna · 2017-10-17T06:03:03Z

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.

Flarna · 2017-10-17T07:00:01Z

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.

ghost · 2017-10-17T15:01:15Z

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

akdor1154 · 2017-10-26T23:22:28Z

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

ghost · 2017-10-30T15:10:13Z

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.

RyanCavanaugh · 2017-10-31T20:27:28Z

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

minestarks · 2017-12-12T01:03:58Z

@akdor1154 could you fix the test failures?

minestarks · 2017-12-14T00:11:45Z

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!

typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Oct 13, 2017

akdor1154 force-pushed the strict branch from 2e66bb7 to 90a034b Compare October 13, 2017 05:02

typescript-bot added the The Travis CI build failed label Oct 13, 2017

Flarna reviewed Oct 13, 2017

View reviewed changes

typescript-bot added The Travis CI build failed and removed The Travis CI build failed labels Oct 15, 2017

Flarna approved these changes Oct 16, 2017

View reviewed changes

typescript-bot added the Author Approved label Oct 16, 2017

akdor1154 force-pushed the strict branch from 486b0da to 6fa9552 Compare October 17, 2017 00:50

akdor1154 force-pushed the strict branch 3 times, most recently from a7f3690 to 7761e1b Compare November 1, 2017 22:58

Jarrad Whitaker added 2 commits November 2, 2017 09:58

enable strict null checks, fix use-before-assign in tests, var -> let

527a7c5

NodeModule.parent -> NodeModule | undefined

576d6cb

Jarrad Whitaker added 7 commits November 2, 2017 09:58

assert.throws[Error] -> optional

592dd5d

stream destroy and reuse method signatures

cdad24d

add PATH as a property on process.env (contentious?)

9e77ceb

readline data is ignore if key is specified

dc4f6d4

clusters - worker assertion

ddac835

inspector - optional properties

8ad6564

errbacks -> Error|null

7761e1b

RyanCavanaugh added The Travis CI build failed and removed The Travis CI build failed labels Nov 1, 2017

Flarna mentioned this pull request Nov 5, 2017

Travis CI build timeout #20308

Closed

minestarks closed this Dec 14, 2017

Flarna mentioned this pull request May 15, 2018

[@types/node] SpawnSyncReturns may have null values #25731

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@types/node: enable strictnullchecks, fix resulting issues #20542

@types/node: enable strictnullchecks, fix resulting issues #20542

akdor1154 commented Oct 13, 2017 •

edited

dt-bot commented Oct 13, 2017

akdor1154 commented Oct 13, 2017 •

edited

typescript-bot commented Oct 13, 2017

Flarna Oct 13, 2017

akdor1154 Oct 13, 2017

Flarna Oct 13, 2017

Flarna Oct 13, 2017

Flarna Oct 13, 2017 •

edited

ghost commented Oct 16, 2017

akdor1154 commented Oct 16, 2017 •

edited

ghost commented Oct 16, 2017

akdor1154 commented Oct 16, 2017 •

edited

ghost commented Oct 16, 2017

akdor1154 commented Oct 16, 2017

akdor1154 commented Oct 17, 2017 •

edited

Flarna commented Oct 17, 2017

Flarna commented Oct 17, 2017

ghost commented Oct 17, 2017

akdor1154 commented Oct 26, 2017

ghost commented Oct 30, 2017

RyanCavanaugh commented Oct 31, 2017

minestarks commented Dec 12, 2017

minestarks commented Dec 14, 2017

@types/node: enable strictnullchecks, fix resulting issues #20542

@types/node: enable strictnullchecks, fix resulting issues #20542

Conversation

akdor1154 commented Oct 13, 2017 • edited

dt-bot commented Oct 13, 2017

akdor1154 commented Oct 13, 2017 • edited

typescript-bot commented Oct 13, 2017

Flarna Oct 13, 2017

Choose a reason for hiding this comment

akdor1154 Oct 13, 2017

Choose a reason for hiding this comment

Flarna Oct 13, 2017

Choose a reason for hiding this comment

Flarna Oct 13, 2017

Choose a reason for hiding this comment

Flarna Oct 13, 2017 • edited

Choose a reason for hiding this comment

ghost commented Oct 16, 2017

akdor1154 commented Oct 16, 2017 • edited

ghost commented Oct 16, 2017

akdor1154 commented Oct 16, 2017 • edited

ghost commented Oct 16, 2017

akdor1154 commented Oct 16, 2017

akdor1154 commented Oct 17, 2017 • edited

Flarna commented Oct 17, 2017

Flarna commented Oct 17, 2017

ghost commented Oct 17, 2017

akdor1154 commented Oct 26, 2017

ghost commented Oct 30, 2017

RyanCavanaugh commented Oct 31, 2017

minestarks commented Dec 12, 2017

minestarks commented Dec 14, 2017

akdor1154 commented Oct 13, 2017 •

edited

akdor1154 commented Oct 13, 2017 •

edited

Flarna Oct 13, 2017 •

edited

akdor1154 commented Oct 16, 2017 •

edited

akdor1154 commented Oct 16, 2017 •

edited

akdor1154 commented Oct 17, 2017 •

edited