chore(node): apply lint #29151

SimonSchick · 2018-09-24T14:00:34Z

I changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: Does not apply
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" }.

This applies most lint rules, except ones that would introduce too many breaking changes.
The main changes here are:

Removal of almost all export keywords (http2 is a special case)
line lengths (ugh)
removals of generic that the linter didn't like, this might cause some issues but ideally shouldn't.
All values are not const instead of var, this might be debatable because some may re-write globals, please advise.

This (for now) includes #29147

typescript-bot · 2018-09-24T14:25:01Z

@SimonSchick Thank you for submitting this PR!

🔔 @joeskeen @aznnomness @cspotcode @chrootsu @jeffmay @basarat @barbatus @fullflavedave @orefalo @dagatsoin @birkskyum @ardatan @stefanholzapfel @andrei-markeev @lmachens @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis @eps1lon @Hannes-Magnusson-CK @jkomyno @ajafff @hoo29 @n-e @BrunoScheufler @mohsen1 @KSXGitHub @a-tarasyuk @islishude @r3nya @ZaneHannanAU @ThomasdenH @eyqs @matthieusieben @Morfent @nonAlgebraic @wokim @heycalmdown @stevehipwell @nickiannone @ashleybrener - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

SimonSchick · 2018-09-24T15:08:05Z

Note: Changes caused some other packages to fail, is there any way to test this locally @basarat ?
Notable changes in other packages:
Removed console declaration of meteor since that appears to conflict.
Removed undefined from ParsedUrlQuery as according to the docs this is not even possible.
Add cast to grunt as it's being a special snowflake.

SimonSchick · 2018-09-24T15:37:38Z

Another update:
Removed ServerRequest and ClientResponse as these classes do not exist anymore.
Removed runInDebugContext as it was removed.

typescript-bot · 2018-09-24T16:27:17Z

@SimonSchick The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2018-09-24T17:26:42Z

@SimonSchick The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Flarna · 2018-09-24T20:55:00Z

types/node/index.d.ts

        maxKeys?: number;
        decodeURIComponent?: Function;
    }

-    interface ParsedUrlQuery { [key: string]: string | string[] | undefined; }
+    interface ParsedUrlQuery { [key: string]: string | string[]; }


The reason why | undefined was used here is that this allows you to augment ParsedUrlQuery like this in your application to easier access properties:

my_key?: string, my_arr_key?: string[] }

if | undefined is not there it's only possible to specify mandatory properties.

I'm not following, can you provide an example maybe?

let result: querystring.ParsedUrlQuery; const a: { a?: string; } = result;

works just fine in this case (pulled from test).

If you are talking about augmenting the interface through declaration merging however:
I think it's a really really bad practise to do that as you can have various definitions of query string parameters for all sorts of purposes, you should cast to your desired type instead.

See the http.IncomingHttpHeaders which holds an index signature on the one hand and named optional properties on the other hand to narrow types for known/specified headers. This works only if | undefined is added in the index signature.

Users could do the same by augmenting ParsedUrlQuery but I agree that this is different and not really an intended usecase.

Flarna · 2018-09-24T20:56:35Z

types/node/index.d.ts

+    function request(url: string | URL, options: RequestOptions, callback?: (res: IncomingMessage) => void): ClientRequest;
+    function get(options: RequestOptions | string | URL, callback?: (res: IncomingMessage) => void): ClientRequest;
+    function get(url: string | URL, options: RequestOptions, callback?: (res: IncomingMessage) => void): ClientRequest;
+    const globalAgent: Agent;


globalAgent is not const.

Seems that way, however changing it on the fly could break references.
I will change it to let.

Flarna · 2018-09-24T20:58:40Z

types/node/index.d.ts


 // Same as module.exports
-declare var exports: any;
-declare var SlowBuffer: {
+declare const exports: any;


is this really const?

Usually exports is passed in by the nodejs module wrapper as an argument.
It is debatable if this is const but assigning it effectively breaks exports as you are changing the reference, you should always set module.exports

Yes, right. adding properties to exports is the usecase, not assigning to it.

Flarna · 2018-09-24T21:01:46Z

types/node/index.d.ts

+    function request(url: string | URL, options: RequestOptions, callback?: (res: http.IncomingMessage) => void): http.ClientRequest;
+    function get(options: RequestOptions | string | URL, callback?: (res: http.IncomingMessage) => void): http.ClientRequest;
+    function get(url: string | URL, options: RequestOptions, callback?: (res: http.IncomingMessage) => void): http.ClientRequest;
+    const globalAgent: Agent;


I think this is not const. It may be a valid usecase to modify the global agent in an application.

Flarna · 2018-09-24T21:07:38Z

Quite a big changeset so maybe I haven't overlooked something...
Please note that inspector.d.ts is a generated file generated via the scripts in types/node/scripts/generate-inspector therefore I think these scripts should be adapted instead of inspector.d.ts.

typescript-bot · 2018-09-24T21:23:16Z

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

SimonSchick · 2018-09-24T22:22:25Z

@Flarna I am aware it's generated but the link to the generator is dead so I dunno where it is 😛

islishude

Remove so much export,I don't think that's the right thing to do. there may be compatibility problems.

types/node/index.d.ts

typescript-bot · 2018-09-25T01:23:48Z

@SimonSchick One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

SimonSchick · 2018-09-25T01:25:37Z

@islishude The linter made it clear these export keywords were redundant, (except for http2).

Flarna · 2018-09-25T10:23:56Z

@SimonSchick Isn't the generator in the subfolder types/node/scripts/generate-inspector?
I never touched the inspector types so don't know. @kjin I think you did the last update of inspector types. Could you please help regarding the generator?

SimonSchick · 2018-09-25T10:30:24Z

@Flarna my bad, I had scripts hidden in my vscode, some config leak I suppose 😛

SimonSchick · 2018-09-25T11:46:03Z

Update:
Updated generator to produce lint conform code (omit redundant namespacing, wrap doc lines, wrap long method lines).

SimonSchick · 2018-09-25T18:50:32Z

Hey @basarat I think this is ready.

Flarna · 2018-09-25T19:10:08Z

types/node/index.d.ts

-    export function parse<T extends {}>(str: string, sep?: string, eq?: string, options?: ParseOptions): T;
-    export function escape(str: string): string;
-    export function unescape(str: string): string;
+    function stringify(obj?: {}, sep?: string, eq?: string, options?: StringifyOptions): string;


I think the first argument is mandatory.

Forgive the ghetto windows shell:

C:\Users\Simon>node -v v10.10.0 C:\Users\Simon>node > querystring.stringify(undefined) '' > querystring.stringify() '' >

ok, behavior of node and doc differs here.

amcasey · 2018-09-25T23:01:33Z

@Flarna I think you're still reviewing - please let me know if/when you're satisfied.

ThomasdenH

The only comment I have is the usage of <any> casting instead of using as any. This is the recommended syntax.

ThomasdenH · 2018-09-26T09:11:56Z

types/grunt/grunt-tests.ts

 // exports should work same as module.exports
-exports = (grunt: IGrunt) => {
+// assigning exports is an error in node, hence the cast
+(<any>global).exports = (grunt: IGrunt) => {


Isn't the as syntax recommended now?

ThomasdenH · 2018-09-26T09:21:25Z

types/node/node-tests.ts

-        let x: NodeModule;
-        let y: NodeModule;
+        const x: NodeModule = <any> {};
+        const y: NodeModule = <any> {};


Same here. as is recommended instead.

I suppose, not a big change, but this should really be added to the lint config if it's so important @basarat

ThomasdenH · 2018-09-26T09:24:19Z

types/node/node-tests.ts

    {
-        let http2Stream: http2.Http2Stream;
-        let duplex: stream.Duplex = http2Stream;
+        const http2Stream: http2.Http2Stream = <any> {};


Casting without as.

ThomasdenH · 2018-09-26T09:24:35Z

types/node/node-tests.ts


        // ClientHttp2Stream
-        let clientHttp2Stream: http2.ClientHttp2Stream;
+        const clientHttp2Stream: http2.ClientHttp2Stream = <any> {};


Casting without as.

ThomasdenH · 2018-09-26T09:25:01Z

types/node/node-tests.ts

-        let serverHttp2Stream: http2.ServerHttp2Stream;
-        let headers: http2.OutgoingHttpHeaders;
+        const serverHttp2Stream: http2.ServerHttp2Stream = <any> {};
+        const headers: http2.OutgoingHttpHeaders = {};


Casting without as.

ThomasdenH · 2018-09-26T09:25:27Z

types/node/node-tests.ts

+        const onConnectHandler = (session: http2.Http2Session, socket: net.Socket) => {};

-        let serverHttp2Session: http2.ServerHttp2Session;
+        const serverHttp2Session: http2.ServerHttp2Session = <any> {};


Casting without as.

ThomasdenH · 2018-09-26T09:26:46Z

types/oauth/oauth-tests.ts

        /** Github sends auth code so that access_token can be obtained */
        /** To obtain and parse code='...' from code?code='...' */
-        const qsObj = qs.parse<{[key: string]: string}>(p[1].split('?')[1]);
+        const qsObj = <{ code: string }> qs.parse(p[1].split('?')[1]);


Casting without as.

ThomasdenH · 2018-09-26T12:16:49Z

Not sure if you saw, but there are still these instances of casting with <>;

types/grunt/grunt-tests.ts:17
types/oauth/oauth-tests.ts:47

SimonSchick · 2018-09-26T12:57:17Z

@ThomasdenH I appreciate the thoroughness but I don't think we should be this nit-picky about this.
I will gladly follow everything that is standartized in the lint config, maybe you should request a change to be made there.

ThomasdenH · 2018-09-26T13:00:33Z

You're probably right. Still, since the PR is about style anyway I think it's a good idea. Anyway, looks good to me now!

chore(node): apply lint

352f2a6

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Sep 24, 2018

SimonSchick requested a review from basarat as a code owner September 24, 2018 15:04

fixup! chore(node): apply lint

2b7def6

fixup! fixup! chore(node): apply lint

93819a9

typescript-bot added The Travis CI build failed and removed Awaiting reviewer feedback labels Sep 24, 2018

fixup! fixup! fixup! chore(node): apply lint

5cf5316

typescript-bot added Awaiting reviewer feedback and removed The Travis CI build failed labels Sep 24, 2018

Flarna reviewed Sep 24, 2018

View reviewed changes

jeffmay approved these changes Sep 24, 2018

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express and removed Awaiting reviewer feedback labels Sep 24, 2018

fixup! fixup! fixup! fixup! chore(node): apply lint

c487b88

islishude suggested changes Sep 25, 2018

View reviewed changes

types/node/index.d.ts Show resolved Hide resolved

typescript-bot removed the Owner Approved A listed owner of this package signed off on the pull request. label Sep 25, 2018

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Merge:Express labels Sep 25, 2018

islishude approved these changes Sep 25, 2018

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express labels Sep 25, 2018

fixup! fixup! fixup! fixup! fixup! chore(node): apply lint

7150c8b

Flarna reviewed Sep 25, 2018

View reviewed changes

Flarna approved these changes Sep 26, 2018

View reviewed changes

ThomasdenH reviewed Sep 26, 2018

View reviewed changes

fixup! fixup! fixup! fixup! fixup! fixup! chore(node): apply lint

2c2446d

fixup! fixup! fixup! fixup! fixup! fixup! fixup! chore(node): apply lint

a2bdb78

ThomasdenH approved these changes Sep 26, 2018

View reviewed changes

amcasey merged commit dfe659d into DefinitelyTyped:master Sep 26, 2018

sandersn mentioned this pull request Oct 2, 2018

🤖 User test baselines have changed microsoft/TypeScript#27498

Merged

chore(node): apply lint #29151

chore(node): apply lint #29151

Uh oh!

Conversation

SimonSchick commented Sep 24, 2018

Uh oh!

typescript-bot commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonSchick commented Sep 24, 2018

Uh oh!

SimonSchick commented Sep 24, 2018

Uh oh!

typescript-bot commented Sep 24, 2018

Uh oh!

typescript-bot commented Sep 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flarna commented Sep 24, 2018

Uh oh!

typescript-bot commented Sep 24, 2018

Uh oh!

SimonSchick commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

islishude left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

typescript-bot commented Sep 25, 2018

Uh oh!

SimonSchick commented Sep 25, 2018

Uh oh!

Flarna commented Sep 25, 2018

Uh oh!

SimonSchick commented Sep 25, 2018

Uh oh!

SimonSchick commented Sep 25, 2018

Uh oh!

SimonSchick commented Sep 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcasey commented Sep 25, 2018

Uh oh!

ThomasdenH left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

typescript-bot commented Sep 24, 2018 •

edited

Loading

SimonSchick commented Sep 24, 2018 •

edited

Loading