feat(node): v12.7 by SimonSchick · Pull Request #37217 · DefinitelyTyped/DefinitelyTyped

SimonSchick · 2019-07-29T21:03:09Z

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.)
Add or edit tests to reflect the change. (Run with npm test.)
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: https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#12.7.0
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

Also:
Fixes #37035

typescript-bot · 2019-07-29T21:22:52Z

@SimonSchick Thank you for submitting this PR!

🔔 @microsoft @DefinitelyTyped @jkomyno @a-tarasyuk @alvis @r3nya @btoueg @BrunoScheufler @smac89 @tellnes @Touffy @DeividasBakanas @eyqs @Flarna @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @matthieusieben @mohsen1 @n-e @octo-sniffle @parambirs @eps1lon @ThomasdenH @WilcoBakker @wwwy3y3 @ZaneHannanAU @samuela @kuehlein @j-oliveras @bhongy @chyzwar - 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.

typescript-bot · 2019-07-29T21:22:54Z

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!

typescript-bot · 2019-07-29T22:22:48Z

@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 · 2019-07-29T23:10:50Z

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#37217	diff
Batch compilation
Type count	20162	20435	+1.4%
Assignability cache size	38134	38358	+0.6%
Subtype cache size	2460	2624	+6.7%
Identity cache size	19	19	0.0%

Language service
Samples taken	4394	2501	-43.1%
Identifiers in tests	10617	10775	+1.5%
`getCompletionsAtPosition`
Mean duration (ms)	830.4	790.9	-4.8%
Median duration (ms)	827.6	784.2	-5.2%
Mean CV		5.5%
Worst duration (ms)	1259.2	1399.1	+11.1%
Worst identifier	encoding	err
`getQuickInfoAtPosition`
Mean duration (ms)	796.0	770.8	-3.2%
Median duration (ms)	776.5	763.7	-1.7%
Mean CV		5.9%
Worst duration (ms)	1154.5	1367.3	+18.4%
Worst identifier	inspector	fs

System information
Node version	v10.15.3	v10.16.0
CPU count	2	2
CPU speed	2.397 GHz	2.394 GHz
CPU model	Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz	Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz
CPU Architecture	x64	x64
Memory	6.8 GiB	6.8 GiB
Platform	linux	linux
Release	4.15.0-1041-azure	4.15.0-1047-azure

First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt.

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

Flarna · 2019-07-30T16:39:02Z

types/node/dns.d.ts

-        function __promisify__(hostname: string, options?: LookupOneOptions | number): Promise<{ address: string, family: number }>;
-        function __promisify__(hostname: string, options?: LookupOptions | number): Promise<{ address: string | LookupAddress[], family?: number }>;
+        function __promisify__(hostname: string, options: LookupAllOptions): Promise<LookupAddress[]>;
+        function __promisify__(hostname: string, options?: LookupOneOptions | number): Promise<LookupAddress[]>;


I'm quite sure that the LookupOneOptions and LookupOptions variant is correct, see the last doc line for lookup at https://nodejs.org/dist/latest-v12.x/docs/api/dns.html#dns_dns_lookup_hostname_options_callback
Maybe the | number for the LookupOptions variant should be removed as this is better covered by the LookupOneOptions variant.

I'm not following, please post the suggested diff.

First variant as you suggested, second as it was before and third a combination of both:

function __promisify__(hostname: string, options: LookupAllOptions): Promise<LookupAddress[]>; function __promisify__(hostname: string, options?: LookupOneOptions | number): Promise<{ address: string, family: number }>; function __promisify__(hostname: string, options: LookupOptions ): Promise<{ address: string, family: number } | LookupAddress[]>;

Flarna · 2019-07-30T16:45:12Z

types/node/http.d.ts

        setSocketKeepAlive(enable?: boolean, initialDelay?: number): void;
+
+        addListener(event: 'information', listener: (info: InformationEvent) => void): this;
+        addListener(event: string | symbol, listener: (...args: any[]) => void): this;


Doesn't this hide the addListener,... variants from base class? sure, there is the generic variant so nothing is broken but now only the information event is typed.
On the other hand copy pasting all the other definitions is also not nice...

Well, that's a problem, not only in this place but already existing ones as well.
I will use declaration merging instead.

Which.. apparently doesn't work anymore, this is going to become annoying.

With newer typescript versions this could be improved to define lists of types and reuse them at several places... but we have to stick on 2.0/2.1...

SimonSchick · 2019-07-30T17:40:26Z

@rbuckton any suggestions on how to handle overloads with inheritance?

SimonSchick · 2019-07-30T18:56:12Z

@Flarna I will be n/a for a day or 2, I gave you push rights to my fork for now, feel free to fix things as you see fit.

typescript-bot · 2019-07-30T21:27:04Z

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

add http.response.writableFinished add tests fix lint

Flarna · 2019-07-30T22:41:26Z

I committed some fixes. I tried also to add Readable.readableObjectMode and Readable.readableEncoding but this resulted in other problems so I removed it.

I have not touched the events of ClientRequest, maybe @rbuckton or @weswigham have a good idea. See also microsoft/TypeScript#30612

weswigham · 2019-07-30T23:13:46Z

any suggestions on how to handle overloads with inheritance?

Copy them all, tbh. Overloads don't inherit well - you either get all of them or none.

SimonSchick · 2019-07-30T23:44:26Z

@weswigham that's not really a sustainable solution, it solves the problem for now but probably creates more later and I'm curious if the TS/DT team knows a solution to that :)

typescript-bot · 2019-07-31T00:01:27Z

Updated numbers for you here from 53d63e4:

Comparison details 📊

	master	#37217	diff
Batch compilation
Type count	20162	20437	+1.4%
Assignability cache size	38134	38362	+0.6%
Subtype cache size	2460	2624	+6.7%
Identity cache size	19	19	0.0%

Language service
Samples taken	4394	2501	-43.1%
Identifiers in tests	10617	10787	+1.6%
`getCompletionsAtPosition`
Mean duration (ms)	830.4	786.8	-5.2%
Median duration (ms)	827.6	780.4	-5.7%
Mean CV		5.5%
Worst duration (ms)	1259.2	1187.2	-5.7%
Worst identifier	encoding	constants
`getQuickInfoAtPosition`
Mean duration (ms)	796.0	758.6	-4.7%
Median duration (ms)	776.5	749.3	-3.5%
Mean CV		6.1%
Worst duration (ms)	1154.5	1191.4	+3.2%
Worst identifier	inspector	Buffer

System information
Node version	v10.15.3	v10.16.0
CPU count	2	2
CPU speed	2.397 GHz	2.294 GHz
CPU model	Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz	Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz
CPU Architecture	x64	x64
Memory	6.8 GiB	6.8 GiB
Platform	linux	linux
Release	4.15.0-1041-azure	4.15.0-1047-azure

First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt.

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

weswigham · 2019-07-31T00:12:03Z

it solves the problem for now but probably creates more later and I'm curious if the TS/DT team knows a solution to that :)

Yeah, I know. You can do something like methodName: BaseType["methodName"] & {(type: "newOverload"): void}, but that's kinda ugly, too.

rbuckton · 2019-07-31T03:58:30Z

I've had a number of discussions with @DanielRosenwasser about ways to improve typing for NodeJS (and DOM) events, but we haven't settled on a good solution yet.

Flarna

events from base class of ClientRequest need to be copied.

typescript-bot · 2019-07-31T08:25:14Z

@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 · 2019-07-31T16:43:31Z

@Flarna I think one option here would be to extract all event into separate interfaces and then decl merge them together, this way we could keep those reusable, still kinda hacky.

Flarna · 2019-08-01T19:18:18Z

@SimonSchick I just copy pasted once again and added also the other events documented for ClientRequest. I think it's better to cleanup the event stuff in a dedicated PR once for all but I fear the limitation to support TS 2.0 will render this anyway to fail.

Flarna

.

Flarna · 2019-08-01T19:21:05Z

I have the feeling that I self approved this PR now but I have no idea how to avoid this...

typescript-bot · 2019-08-01T21:22:19Z

Updated numbers for you here from 83dd75a:

Comparison details 📊

	master	#37217	diff
Batch compilation
Type count	20162	20506	+1.7%
Assignability cache size	38134	38440	+0.8%
Subtype cache size	2460	2624	+6.7%
Identity cache size	19	19	0.0%

Language service
Samples taken	4394	2501	-43.1%
Identifiers in tests	10617	10787	+1.6%
`getCompletionsAtPosition`
Mean duration (ms)	830.4	860.3	+3.6%
Median duration (ms)	827.6	844.6	+2.1%
Mean CV		4.7%
Worst duration (ms)	1259.2	1200.5	-4.7%
Worst identifier	encoding	opts
`getQuickInfoAtPosition`
Mean duration (ms)	796.0	813.0	+2.1%
Median duration (ms)	776.5	793.3	+2.2%
Mean CV		5.2%
Worst duration (ms)	1154.5	1266.2	+9.7%
Worst identifier	inspector	num

System information
Node version	v10.15.3	v10.16.0
CPU count	2	2
CPU speed	2.397 GHz	2.397 GHz
CPU model	Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz	Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz
CPU Architecture	x64	x64
Memory	6.8 GiB	6.8 GiB
Platform	linux	linux
Release	4.15.0-1041-azure	4.15.0-1047-azure

First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt.

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.

sandersn · 2019-08-05T17:46:06Z

@Flarna so is this ready to go? I got confused by your "self-approved" comment whether you meant to sign off or not.

Flarna · 2019-08-05T20:10:37Z

@sandersn Yes, it's ready. Thanks!

typescript-bot · 2019-08-06T22:37:36Z

I just published @types/node@12.7.0 to npm.

stephank · 2019-08-12T11:26:16Z

types/node/test/readline.ts

    const x = 1;
    const y = 1;

-    readline.cursorTo(strm, x);


This line was in place to test the change in #17684, which has now been undone.

Current Node.js still appears to accept y as an optional parameter, but they also never updated the docs. (Doesn't look like the PR was ever created.)

I think we should start with a PR for node otherwise such mismatches will confuse people again and again. Maybe I find some time the next days to do this.

For the record, I have just opened #37630. Hope this helps!

I created a PR for node: nodejs/node#29128

* feat(node): v12.7 * correct dns.lookup add http.response.writableFinished add tests fix lint * add events to http.ClientRequest

feat(node): v12.7

39932af

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback Author is Owner The author of this PR is a listed owner of the package. labels Jul 29, 2019

typescript-bot added The Travis CI build failed and removed Awaiting reviewer feedback labels Jul 29, 2019

Flarna reviewed Jul 30, 2019

View reviewed changes

correct dns.lookup

3a9b0d5

add http.response.writableFinished add tests fix lint

typescript-bot added Awaiting reviewer feedback and removed The Travis CI build failed labels Jul 30, 2019

Flarna suggested changes Jul 31, 2019

View reviewed changes

typescript-bot removed Awaiting reviewer feedback labels Jul 31, 2019

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jul 31, 2019

add events to http.ClientRequest

63d5902

Flarna reviewed Aug 1, 2019

View reviewed changes

Flarna approved these changes Aug 1, 2019

View reviewed changes

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

BrunoScheufler approved these changes Aug 2, 2019

View reviewed changes

sandersn merged commit 3f6627e into DefinitelyTyped:master Aug 6, 2019

SimonSchick deleted the feat/node-v12.7 branch August 7, 2019 00:30

stephank reviewed Aug 12, 2019

View reviewed changes

iRON5 pushed a commit to iRON5/DefinitelyTyped that referenced this pull request Aug 13, 2019

feat(node): v12.7 (DefinitelyTyped#37217)

685be1c

* feat(node): v12.7 * correct dns.lookup add http.response.writableFinished add tests fix lint * add events to http.ClientRequest

astorije mentioned this pull request Aug 14, 2019

[@types/node] Fix cursorTo requiring a y parameter #37630

Merged

7 tasks

Conversation

SimonSchick commented Jul 29, 2019

Uh oh!

typescript-bot commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jul 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flarna Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

SimonSchick commented Jul 30, 2019

Uh oh!

SimonSchick commented Jul 30, 2019

Uh oh!

typescript-bot commented Jul 30, 2019

Uh oh!

Flarna commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Jul 30, 2019

Uh oh!

SimonSchick commented Jul 30, 2019

Uh oh!

typescript-bot commented Jul 31, 2019

Uh oh!

weswigham commented Jul 31, 2019

Uh oh!

rbuckton commented Jul 31, 2019

Uh oh!

Flarna left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Jul 31, 2019

Uh oh!

SimonSchick commented Jul 31, 2019

Uh oh!

Flarna commented Aug 1, 2019

Uh oh!

Flarna left a comment

Choose a reason for hiding this comment

Uh oh!

Flarna commented Aug 1, 2019

Uh oh!

typescript-bot commented Aug 1, 2019

Uh oh!

sandersn commented Aug 5, 2019

Uh oh!

Flarna commented Aug 5, 2019

Uh oh!

typescript-bot commented Aug 6, 2019

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!

Reviewers

typescript-bot commented Jul 29, 2019 •

edited

Loading

typescript-bot commented Jul 29, 2019 •

edited

Loading

typescript-bot commented Jul 29, 2019 •

edited

Loading

Flarna Jul 30, 2019 •

edited

Loading

Flarna commented Jul 30, 2019 •

edited

Loading