-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Provide http server port directly from _server #1294
Conversation
lib/websocket-server.js
Outdated
* Upgrade the connection to WebSocket. | ||
* @public | ||
*/ | ||
port () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of thoughts:
- It's probably better to call this
address
and returnthis._server.address()
. This will handle the case where the HTTP server is closed. WebSocketServer
could be initialized innoServer
mode. In this casethis._server
isnull
. What should we return? Returningnull
doesn't make sense as it's possible that the external server is actually bound and listening for connections. Throwing an error is probably the best option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
understood. Thank for the guide. :)
@lpinca Can we Merge ? :) |
lib/websocket-server.js
Outdated
@@ -275,6 +275,16 @@ class WebSocketServer extends EventEmitter { | |||
|
|||
cb(ws); | |||
} | |||
|
|||
/** | |||
* Provide address of server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a better description and specify what is returned via @return ...
. Copying https://nodejs.org/dist/latest/docs/api/net.html#net_server_address is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put the description on this line and wrap lines at 80 chars.
/**
* Returns the bound address, the address family name, and port of the server
* as reported by the operating system if listening on an IP socket.
* If the server is listening on a pipe or UNIX domain socket, the name is
* returned as a string.
*
* @return {(Object|String|null)} The address of the server
* @public
*/
lib/websocket-server.js
Outdated
* @public | ||
*/ | ||
address () { | ||
if (!this._server) throw new Error(`Server was not initialized`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use this.options.noServer
as we need to throw only when operating in "noServer" mode.
if (this.options.noServer) {
throw new Error('This server is operating in "noServer" mode');
}
lib/websocket-server.js
Outdated
*/ | ||
address () { | ||
if (!this._server) throw new Error(`Server was not initialized`); | ||
console.log(this._server.address()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace this line with
if (!this._server) return null;
This will handle the case where address()
is called after close()
.
test/websocket-server.test.js
Outdated
@@ -724,4 +724,24 @@ describe('WebSocketServer', function () { | |||
}); | |||
}); | |||
}); | |||
|
|||
describe('Server address', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this block after describe('#ctor', ...);
and use '#address'
as title
lib/websocket-server.js
Outdated
@@ -275,6 +275,16 @@ class WebSocketServer extends EventEmitter { | |||
|
|||
cb(ws); | |||
} | |||
|
|||
/** | |||
* Provide address of server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put the description on this line and wrap lines at 80 chars.
/**
* Returns the bound address, the address family name, and port of the server
* as reported by the operating system if listening on an IP socket.
* If the server is listening on a pipe or UNIX domain socket, the name is
* returned as a string.
*
* @return {(Object|String|null)} The address of the server
* @public
*/
test/websocket-server.test.js
Outdated
@@ -137,6 +137,25 @@ describe('WebSocketServer', function () { | |||
}); | |||
}); | |||
|
|||
describe('#address', function () { | |||
it('can provide server address', function (done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'returns the address of the server'
test/websocket-server.test.js
Outdated
it('can provide server address', function (done) { | ||
const wss = new WebSocket.Server({ port: 0 }, () => { | ||
const addr = wss.address(); | ||
assert.deepEqual(wss._server.address(), addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deepStrictEqual
test/websocket-server.test.js
Outdated
}); | ||
}); | ||
|
||
it(`will thrown exception if server was closed`, function (done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'throws an error when operating in "noServer" mode'
test/websocket-server.test.js
Outdated
}); | ||
|
||
it(`will thrown exception if server was closed`, function (done) { | ||
const wss = new WebSocket.Server({ port: 0, noServer: true }, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add the port
option it will create a server. We don't want that.
it('throws an error when operating in "noServer" mode', function () {
const wss = new WebSocket.Server({ noServer: true });
assert.throws(
() => wss.address(),
/^Error: This server is operating in "noServer" mode$/
);
});
test/websocket-server.test.js
Outdated
}, /This server is operating in "noServer" mode/); | ||
wss.close(done); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new test for the address()
after close()
case where null
is returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okidokie
We also need to add documentation for this method in https://github.com/websockets/ws/blob/master/doc/ws.md |
test/websocket-server.test.js
Outdated
it('returns the address of the server', function (done) { | ||
const wss = new WebSocket.Server({ port: 0 }, () => { | ||
const addr = wss.address(); | ||
assert.deepStrictEqual(wss._server.address(), addr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being picky, we are getting closer. Can you please swap the arguments? actual
first expected
second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use strictEqual
here as well.
test/websocket-server.test.js
Outdated
const wss = new WebSocket.Server({ noServer: true }, () => { | ||
assert.throws(() => { | ||
wss.address(); | ||
}, /This server is operating in "noServer" mode/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use /^Error: This server is operating in "noServer" mode$/
so we also validate the type of the error.
test/websocket-server.test.js
Outdated
it('returns null when called after server closed', function (done) { | ||
const wss = new WebSocket.Server({ port: 0 }, () => { | ||
wss.close(); | ||
assert.deepStrictEqual(null, wss.address()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please swap the arguments and use strictEqual
not deep.
It seems like code coverage doesn't see the cover of some test it('throws an error when operating in "noServer" mode', function () {
const wss = new WebSocket.Server({ noServer: true }, () => {
assert.throws(() => {
wss.address();
}, /^Error: This server is operating in "noServer" mode$/);
});
}); |
Thank you! |
Hey, Thank you so much to complete this PR (you rewrite it all haha) |
Provide http server port directly from _server
Do I need to write test or anything else to complete this PR ?
Or better way to get port if I bind with 0
Thanks