Updated Node.js dgram.send() signature with recent changes in Node.js API by nebrius · Pull Request #25664 · DefinitelyTyped/DefinitelyTyped

nebrius · 2018-05-09T23:33:22Z

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).
Provide a URL to documentation or source code which provides context for the suggested changes: https://nodejs.org/dist/latest-v8.x/docs/api/dgram.html#dgram_socket_send_msg_offset_length_port_address_callback (see history section)
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" }.

typescript-bot · 2018-05-10T00:22:37Z

@nebrius Thank you for submitting this PR!

🔔 @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis @OliverJAsh @eps1lon @Hannes-Magnusson-CK @jkomyno @hoo29 @n-e @ajafff - 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.

Flarna · 2018-05-10T19:48:33Z

    export class Socket extends events.EventEmitter {
-        send(msg: Buffer | String | any[], port: number, address: string, callback?: (error: Error | null, bytes: number) => void): void;
-        send(msg: Buffer | String | any[], offset: number, length: number, port: number, address: string, callback?: (error: Error | null, bytes: number) => void): void;
+        send(msg: Buffer | String | Uint8Array | any[], port: number, address: string, callback?: (error: Error | null, bytes: number) => void): void;


I think it should be string instead String here.
According to docs address is optional.

Flarna · 2018-05-10T19:49:49Z

-        send(msg: Buffer | String | any[], port: number, address: string, callback?: (error: Error | null, bytes: number) => void): void;
-        send(msg: Buffer | String | any[], offset: number, length: number, port: number, address: string, callback?: (error: Error | null, bytes: number) => void): void;
+        send(msg: Buffer | String | Uint8Array | any[], port: number, address: string, callback?: (error: Error | null, bytes: number) => void): void;
+        send(msg: Buffer | String | Uint8Array | any[], offset: number, length: number, port: number, address: string, callback?: (error: Error | null, bytes: number) => void): void;


docs tell If msg is an array, offset and length must not be specified. therefore I think | any[] should be removed here.

Flarna

Please apply the change also to Node 10.

typescript-bot · 2018-05-10T20:23:11Z

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

nebrius · 2018-05-10T20:28:29Z

Please apply the change also to Node 10.

Whoops, my local branch was just barely out of date enough to not have the v10 docs yet, will fix. Everything else makes sense too.

nebrius · 2018-05-10T20:39:04Z

Looks like I spoke too soon, there doesn't appear to be v10 docs yet: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/node

Flarna · 2018-05-10T20:45:35Z

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/index.d.ts is latest which is Node 10 since ~2 weeks.

nebrius · 2018-05-10T20:50:16Z

Ohhhh, thanks, I didn't pick up that the root index.d.ts was the most recent version.

nebrius · 2018-05-10T21:02:02Z

All comments should be addressed. Note: I also marked the address parameter optional to conform to the docs.

typescript-bot · 2018-05-10T21:23:41Z

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

nebrius · 2018-05-10T21:29:20Z

Travis failure seems to be spurious?

Flarna · 2018-05-10T22:14:39Z

I agree that CI failure is unrelated. I have seen this recently in #25597 where retrigger CI (by closing and reopening) "solved" this - even log showed the following (see https://api.travis-ci.org/v3/job/376465088/log.txt):

redux-form/v6 OK
The 'no-void-expression' rule threw an error in '/home/travis/build/DefinitelyTyped/DefinitelyTyped/types/redux-little-router/redux-little-router-tests.tsx':
RangeError: Maximum call stack size exceeded
    at isPossiblyReferencedInConditionalType (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/typescript/lib/typescript.js:34309:17)
    at /home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/typescript/lib/typescript.js:34328:146
    at Object.filter (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/typescript/lib/typescript.js:1969:31)
    at getTypeFromConditionalTypeNode (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/typescript/lib/typescript.js:34328:92)
    at getTypeFromTypeNode (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/typescript/lib/typescript.js:34695:28)
    at getTemplateTypeFromMappedType (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/typescript/lib/typescript.js:32020:52)
    at substituteIndexedMappedType (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/typescript/lib/typescript.js:34165:36)
    at getSimplifiedIndexedAccessType (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/typescript/lib/typescript.js:34157:28)
    at getSimplifiedType (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/typescript/lib/typescript.js:34110:63)
    at isRelatedTo (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/typescript/lib/typescript.js:35594:30)

@Andy-MS Any hint regarding this spurious failures?

typescript-bot · 2018-05-10T22:23:01Z

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!

ghost · 2018-05-10T22:41:17Z

I'll look into that soon -- need to improve the error message from "something went wrong"

… API (DefinitelyTyped#25664) * Updated “send” signature with recent changse * Fixed some other issues with send() signature and added Node 10

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

Flarna reviewed May 10, 2018

View reviewed changes

Flarna suggested changes May 10, 2018

View reviewed changes

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback labels May 10, 2018

Updated “send” signature with recent changse

0a62962

Fixed some other issues with send() signature and added Node 10

2168b3f

nebrius force-pushed the nodejs-dgram-send branch from 83720f7 to 2168b3f Compare May 10, 2018 21:00

typescript-bot added The Travis CI build failed Revision needed This PR needs code changes before it can be merged. labels May 10, 2018

Flarna approved these changes May 10, 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 The Travis CI build failed labels May 10, 2018

ghost merged commit 8cd6bba into DefinitelyTyped:master May 10, 2018

ghost mentioned this pull request May 10, 2018

Maximum call stack size exceeded microsoft/TypeScript#24043

Closed

alecmev mentioned this pull request May 11, 2018

react-redux: Fix Provider typing problem discussed in #25321 #25709

Closed

9 tasks

alecmev mentioned this pull request Jun 7, 2018

react-redux: Fix Provider typing problem discussed in #25321 #26360

Merged

9 tasks

This pull request was closed.

Conversation

nebrius commented May 9, 2018

Uh oh!

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

Uh oh!

Uh oh!

Flarna May 10, 2018

Choose a reason for hiding this comment

Uh oh!

Flarna May 10, 2018

Choose a reason for hiding this comment

Uh oh!

Flarna left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented May 10, 2018

Uh oh!

nebrius commented May 10, 2018

Uh oh!

nebrius commented May 10, 2018

Uh oh!

Flarna commented May 10, 2018

Uh oh!

nebrius commented May 10, 2018

Uh oh!

nebrius commented May 10, 2018

Uh oh!

typescript-bot commented May 10, 2018

Uh oh!

nebrius commented May 10, 2018

Uh oh!

Flarna commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented May 10, 2018

Uh oh!

ghost commented May 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

typescript-bot commented May 10, 2018 •

edited

Loading

Flarna commented May 10, 2018 •

edited

Loading