Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose Connector in the public API #906

Merged
merged 11 commits into from
Jul 28, 2021
Merged

Expose Connector in the public API #906

merged 11 commits into from
Jul 28, 2021

Conversation

delvedor
Copy link
Member

@delvedor delvedor commented Jul 27, 2021 •
edited
Loading

As titled. I've also added an example to show how to validate the CA fingerprint.

Related: #904 #905

@delvedor delvedor requested review from mcollina and ronag July 27, 2021 10:05
@delvedor delvedor added enhancement New feature or request semver-minor Features or fixes that will be included in the next semver minor release labels Jul 27, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2021 •
edited
Loading

Codecov Report

Merging #906 (f087fca) into main (6061920) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #906   +/-   ##
=======================================
  Coverage   99.57%   99.57%           
=======================================
  Files          26       26           
  Lines        2132     2132           
=======================================
  Hits         2123     2123           
  Misses          9        9
Impacted Files Coverage Δ
index.js 100.00% <100.00%> (ø)
lib/client.js 99.60% <100.00%> (ø)
lib/core/connect.js 100.00% <100.00%> (ø)
lib/pool.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6061920...f087fca. Read the comment docs.

ronag
ronag requested changes Jul 27, 2021
View reviewed changes
Copy link
Member

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

Actually isn't it better to just expose the factory method instead of the class? The class is just an implementation detail.

@delvedor
Copy link
Member Author

Actually isn't it better to just expose the factory method instead of the class? The class is just an implementation detail.

I like exposing the class, most of the time the user will just need to initialize it, but if needed it can be extended quite easily.

@ronag
Copy link
Member

ronag commented Jul 27, 2021 •
edited
Loading

I like exposing the class, most of the time the user will just need to initialize it, but if needed it can be extended quite easily.

But it being a class doesn't really do anything? Nothing is exposed. It's just a silly way to implement a closure. Connector.connect = Function.call.

ronag
ronag approved these changes Jul 27, 2021
View reviewed changes
Copy link
Member

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

I'd prefer a function but I guess a class is fine...

@delvedor
Copy link
Member Author

Hold on before merging, there might be an issue with the types, checking...

ronag
ronag reviewed Jul 27, 2021
View reviewed changes
test/ca-fingerprint.js Outdated Show resolved Hide resolved
mcollina
mcollina reviewed Jul 27, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Is there a reason why we have a class in the first place? I would just remove it and expose a function.

@ronag
Copy link
Member

ronag commented Jul 27, 2021

Is there a reason why we have a class in the first place? I would just remove it and expose a function.

Nothing other than convenience of implementation.

@delvedor
Copy link
Member Author

Oks, this pr is good to go from my point of view, the types are now correct.

@delvedor delvedor mentioned this pull request Jul 27, 2021
Merged
1 task
@delvedor
Copy link
Member Author

@ronag @mcollina I've changed the connector from class to function, let me know if it's what you had in mind.

mcollina
mcollina requested changes Jul 27, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Awesome work! Could you add docs?

ronag
ronag reviewed Jul 28, 2021
View reviewed changes
examples/ca-fingerprint/index.js
})

function getIssuerCertificate (socket) {
let certificate = socket.getPeerCertificate(true)
Copy link
Member

Choose a reason for hiding this comment

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

Should probably check that socket is a tls.Socket.

ronag
ronag reviewed Jul 28, 2021
View reviewed changes
index.js
@@ -20,6 +21,7 @@ module.exports.Client = Client
module.exports.Pool = Pool
module.exports.Agent = Agent

module.exports.buildConnector = buildConnector
Copy link
Member

@ronag ronag Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Now I see why you preferred a class 😄 . Not all that happy with the name but can't think of anything better atm.

mcollina
mcollina approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

Docs! ;)

@delvedor
Copy link
Member Author

Ready to go!

mcollina
mcollina approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag merged commit 11c2db1 into main Jul 28, 2021
@delvedor delvedor deleted the delvedor/connect-public branch July 28, 2021 13:26
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
@delvedor @crysmags
Expose Connector in the public API (nodejs#906)
b68d4ae 
* Expose Connector in the public API

* Updated test

* Add ca fingerprint example

* Improve types

* Updated test

* Nit

* Make connector a function instead of a class

* Updated test

* Updated examples

* Updated docs

* Updated docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronag ronag ronag approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request semver-minor Features or fixes that will be included in the next semver minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@delvedor @codecov-commenter @ronag @mcollina