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.

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

net: refactor net module to module.exports #11698

Closed

Conversation

claudiorodriguez
Copy link
Contributor

Refactor net module to use the more efficient
module.exports = {} pattern.
Also renames internal "connect" function to "internalConnect"
to avoid collision with exported "connect".

See #11611

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

net

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Mar 5, 2017
@@ -59,7 +59,7 @@ exports.createServer = function(options, connectionListener) {
// connect(port, [host], [cb])
// connect(path, [cb]);
//
exports.connect = exports.createConnection = function() {
function connect() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we just called this createConnection() and added the connect: createConnection alias in the export list. This way we won't have to rename connect to internalConnect everywhere.

Copy link
Contributor Author

@claudiorodriguez claudiorodriguez Mar 5, 2017

Choose a reason for hiding this comment

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

You'd still have a "connect" in the exports (i.e. part of the module's API) and a function named "connect" in the code that is not the same as the export, which is still IMHO confusing. Also there's no change in the public API, no need to rename anything outside of this module. Also references inside the module to a "connect" function that is not the same as the export are confusing.
internalConnect might not be the best name though, I admit.

Copy link
Member

Choose a reason for hiding this comment

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

internalConnect does make grepping easier.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

Refactor net module to use the more efficient
module.exports = {} pattern.
Also renames internal "connect" function to "internalConnect"
to avoid collision with exported "connect".
@claudiorodriguez
Copy link
Contributor Author

Rebased and fixed linting error in lib/net, but there's a bunch of other linter errors in some addons tests in the branch when I run make lint

@joyeecheung
Copy link
Member

Rebased and fixed linting error in lib/net, but there's a bunch of other linter errors in some addons tests in the branch when I run make lint

Those should be unrelated to this PR..

New CI: https://ci.nodejs.org/job/node-test-pull-request/7033/

@@ -979,7 +976,7 @@ function lookupAndConnect(self, options) {
var localAddress = options.localAddress;
var localPort = options.localPort;

if (localAddress && !exports.isIP(localAddress))
if (localAddress && !cares.isIP(localAddress))
Copy link
Member

Choose a reason for hiding this comment

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

Making isIP available as a variable would help simplify here and in module.exports.

@fhinkel
Copy link
Member

fhinkel commented Mar 26, 2017

Landed in 3745a4d.

@fhinkel fhinkel closed this Mar 26, 2017
fhinkel pushed a commit that referenced this pull request Mar 26, 2017
Refactor net module to use the more efficient
module.exports = {} pattern.
Also renames internal "connect" function to "internalConnect"
to avoid collision with exported "connect".

PR-URL: #11698
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
Refactor net module to use the more efficient
module.exports = {} pattern.
Also renames internal "connect" function to "internalConnect"
to avoid collision with exported "connect".

PR-URL: #11698
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
@MylesBorins
Copy link
Contributor

should this be backported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants