Skip to content

Add missing native node tls.checkServerIdentity function#21897

Merged
aozgaa merged 2 commits intoDefinitelyTyped:masterfrom
Hannes-Magnusson-CK:master
Dec 5, 2017
Merged

Add missing native node tls.checkServerIdentity function#21897
aozgaa merged 2 commits intoDefinitelyTyped:masterfrom
Hannes-Magnusson-CK:master

Conversation

@Hannes-Magnusson-CK
Copy link
Copy Markdown
Contributor

@Hannes-Magnusson-CK Hannes-Magnusson-CK commented Dec 1, 2017 •
edited
Loading

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

If changing an existing definition:

@dt-bot
Copy link
Copy Markdown
Member

dt-bot commented Dec 1, 2017

types/node/index.d.ts

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

@typescript-bot typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Dec 1, 2017
@Hannes-Magnusson-CK Hannes-Magnusson-CK mentioned this pull request Dec 1, 2017
Merged
6 tasks
Flarna
Flarna reviewed Dec 3, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@Flarna Flarna left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For completeness I would suggest to update also ConnectionOptions.checkServerIdentity to typeof checkServerIdentity.

@Flarna Flarna mentioned this pull request Dec 3, 2017
Merged
8 tasks
eps1lon
eps1lon reviewed Dec 4, 2017
View reviewed changes
Comment thread types/node/index.d.ts
// TODO: change to `type NodeRequireFunction = (id: string) => any;` in next mayor version.
/* tslint:disable:callable-types */
interface NodeRequireFunction {
/* tslint:disable-next-line:callable-types */
Copy link
Copy Markdown
Collaborator

@eps1lon eps1lon Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate of #21768.

@Hannes-Magnusson-CK
Copy link
Copy Markdown
Contributor Author

Hannes-Magnusson-CK commented Dec 4, 2017

Added CheckServerIdentityFunction as a type and used it in the ConnectionOptions.

Did not remove the lint fix as the linked PR hasn't been merged

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Dec 4, 2017

@Hannes-Magnusson-CK Please fix the failures indicated in the Travis CI log.

@Hannes-Magnusson-CK
Copy link
Copy Markdown
Contributor Author

Hannes-Magnusson-CK commented Dec 4, 2017

Travis is passing. I had a syntax error for a second that was fixed before that comment was added

Flarna
Flarna reviewed Dec 4, 2017
View reviewed changes
Comment thread types/node/index.d.ts Outdated
path?: string;
ALPNProtocols?: Array<string | Buffer>;
checkServerIdentity?: (servername: string, cert: string | Buffer | Array<string | Buffer>) => any;
checkServerIdentity?: CheckServerIdentityFunction;
Copy link
Copy Markdown
Contributor

@Flarna Flarna Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it possible to write checkServerIdentity?: typeof checkServerIdentity here and avoid the extra type CheckServerIdentityFunction?
This would ensure that the function and the property here are always in sync.

Copy link
Copy Markdown
Contributor Author

@Hannes-Magnusson-CK Hannes-Magnusson-CK Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only use of typeof in this file was in the Global interface definition, so it seemed like it wasn't expected syntax to use, so I followed the lead of LookupFunction.

If you think using typeof directly and removing the type declaration is kosher in this file, then I'll fix :D

Copy link
Copy Markdown
Contributor

@Flarna Flarna Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I used a dedicated type for LookupFunction instead typeof dns.lookup is that dns.lookup offers more options that the variant used by sockets and I don't think there is a typeof operand to select just the type of one specific overload.
But here we don't have this special case therefore I would suggest to go for typeof - except someone else (or CI) explains why not.

Copy link
Copy Markdown
Contributor Author

@Hannes-Magnusson-CK Hannes-Magnusson-CK Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Fixed :)

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Dec 4, 2017

Approved by a listed owner. PR ready to merge pending express review by a maintainer.

@aozgaa aozgaa merged commit 476a6fe into DefinitelyTyped:master Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eps1lon eps1lon eps1lon left review comments

+1 more reviewer

@Flarna Flarna Flarna approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Hannes-Magnusson-CK @dt-bot @typescript-bot @eps1lon @Flarna @aozgaa