Provide http server port directly from _server #1294

tnuanchuay · 2018-02-04T17:17:33Z

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

lpinca · 2018-02-04T17:39:45Z

lib/websocket-server.js

+   * Upgrade the connection to WebSocket.
+   * @public
+   */
+  port () {


A couple of thoughts:

It's probably better to call this address and return this._server.address(). This will handle the case where the HTTP server is closed.

WebSocketServer could be initialized in noServer mode. In this case this._server is null. What should we return? Returning null 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.

understood. Thank for the guide. :)

tnuanchuay · 2018-02-05T16:46:39Z

@lpinca Can we Merge ? :)

lpinca · 2018-02-05T16:35:46Z

lib/websocket-server.js

@@ -275,6 +275,16 @@ class WebSocketServer extends EventEmitter {

    cb(ws);
  }
+
+  /**
+   * Provide address of server


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.

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 */

lpinca · 2018-02-05T16:51:44Z

lib/websocket-server.js

+   * @public
+   */
+  address () {
+    if (!this._server) throw new Error(`Server was not initialized`);


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'); }

lpinca · 2018-02-05T16:54:10Z

lib/websocket-server.js

+   */
+  address () {
+    if (!this._server) throw new Error(`Server was not initialized`);
+    console.log(this._server.address());


Replace this line with

if (!this._server) return null;

This will handle the case where address() is called after close().

lpinca · 2018-02-05T16:57:24Z

test/websocket-server.test.js

@@ -724,4 +724,24 @@ describe('WebSocketServer', function () {
      });
    });
  });
+
+  describe('Server address', function () {


Please move this block after describe('#ctor', ...); and use '#address' as title

lpinca · 2018-02-05T17:22:04Z

lib/websocket-server.js

@@ -275,6 +275,16 @@ class WebSocketServer extends EventEmitter {

    cb(ws);
  }
+
+  /**
+   * Provide address of server


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 */

lpinca · 2018-02-05T17:23:43Z

test/websocket-server.test.js

@@ -137,6 +137,25 @@ describe('WebSocketServer', function () {
    });
  });

+  describe('#address', function () {
+    it('can provide server address', function (done) {


'returns the address of the server'

lpinca · 2018-02-05T17:24:01Z

test/websocket-server.test.js

+    it('can provide server address', function (done) {
+      const wss = new WebSocket.Server({ port: 0 }, () => {
+        const addr = wss.address();
+        assert.deepEqual(wss._server.address(), addr);


deepStrictEqual

lpinca · 2018-02-05T17:27:31Z

test/websocket-server.test.js

+      });
+    });
+
+    it(`will thrown exception if server was closed`, function (done) {


'throws an error when operating in "noServer" mode'

lpinca · 2018-02-05T17:31:04Z

test/websocket-server.test.js

+    });
+
+    it(`will thrown exception if server was closed`, function (done) {
+      const wss = new WebSocket.Server({ port: 0, noServer: true }, () => {


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$/ ); });

lpinca · 2018-02-05T17:32:19Z

test/websocket-server.test.js

+        }, /This server is operating in "noServer" mode/);
+        wss.close(done);
+      });
+    });


Add a new test for the address() after close() case where null is returned.

lpinca · 2018-02-05T17:34:40Z

We also need to add documentation for this method in https://github.com/websockets/ws/blob/master/doc/ws.md

lpinca · 2018-02-05T17:47:57Z

test/websocket-server.test.js

+    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);


Sorry for being picky, we are getting closer. Can you please swap the arguments? actual first expected second.

We can use strictEqual here as well.

lpinca · 2018-02-05T17:48:58Z

test/websocket-server.test.js

+      const wss = new WebSocket.Server({ noServer: true }, () => {
+        assert.throws(() => {
+          wss.address();
+        }, /This server is operating in "noServer" mode/);


Please use /^Error: This server is operating in "noServer" mode$/ so we also validate the type of the error.

lpinca · 2018-02-05T17:49:51Z

test/websocket-server.test.js

+    it('returns null when called after server closed', function (done) {
+      const wss = new WebSocket.Server({ port: 0 }, () => {
+        wss.close();
+        assert.deepStrictEqual(null, wss.address());


Please swap the arguments and use strictEqual not deep.

tnuanchuay · 2018-02-05T18:00:37Z

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$/);
      });
    });

lpinca · 2018-02-05T19:48:18Z

Thank you!

tnuanchuay · 2018-02-05T21:02:05Z

Hey, Thank you so much to complete this PR (you rewrite it all haha)
You’re awesome !!

provide http server port directly from _server

8af5c88

lpinca reviewed Feb 4, 2018

View reviewed changes

Tossapon Nuanchuay added 2 commits February 5, 2018 22:25

fix from review

3e955f6

write test to support node v4

3b4f124

lpinca reviewed Feb 5, 2018

View reviewed changes

Tossapon Nuanchuay added 2 commits February 6, 2018 00:08

refactor code follow code review

b4ffa36

test start server mode noServer cann't call address()

66502a0

lpinca reviewed Feb 5, 2018

View reviewed changes

refactor code follow by code review

a0b62e9

lpinca reviewed Feb 5, 2018

View reviewed changes

Tossapon Nuanchuay added 2 commits February 6, 2018 00:52

Update ws.md

df4ff2e

use regex in test, and swap arg in assertion

34b3fb6

lpinca added 2 commits February 5, 2018 20:15

Fix nits

0ef1ae1

Fix nits

cc9309e

lpinca merged commit 8d61fa0 into websockets:master Feb 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide http server port directly from _server #1294

Provide http server port directly from _server #1294

tnuanchuay commented Feb 4, 2018

lpinca Feb 4, 2018

tnuanchuay Feb 5, 2018 •

edited

tnuanchuay commented Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018 •

edited

lpinca Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018

tnuanchuay Feb 5, 2018

lpinca commented Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018

lpinca Feb 5, 2018

tnuanchuay commented Feb 5, 2018

lpinca commented Feb 5, 2018

tnuanchuay commented Feb 5, 2018

Provide http server port directly from _server #1294

Provide http server port directly from _server #1294

Conversation

tnuanchuay commented Feb 4, 2018

Choose a reason for hiding this comment

tnuanchuay Feb 5, 2018 • edited

Choose a reason for hiding this comment

tnuanchuay commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Feb 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnuanchuay commented Feb 5, 2018

lpinca commented Feb 5, 2018

tnuanchuay commented Feb 5, 2018

tnuanchuay Feb 5, 2018 •

edited

lpinca Feb 5, 2018 •

edited