refactor: avoid new public options on class (#3261)

Author: snitin315

lib/Server.js

@@ -14,6 +14,8 @@ const routes = require('./utils/routes');
 const getSocketServerImplementation = require('./utils/getSocketServerImplementation');
 const getCompilerConfigArray = require('./utils/getCompilerConfigArray');
 const setupExitSignals = require('./utils/setupExitSignals');
+const getStatsOption = require('./utils/getStatsOption');
+const getColorsOption = require('./utils/getColorsOption');
 const schema = require('./options.json');
 
 if (!process.env.WEBPACK_SERVE) {
@@ -40,6 +42,7 @@ class Server {
     this.wsHeartbeatInterval = 30000;
 
     normalizeOptions(this.compiler, this.options);
+
     this.applyDevServerPlugin();
 
     this.SocketServerImplementation = getSocketServerImplementation(
@@ -629,8 +632,6 @@ class Server {
   }
 
   showStatus() {
-    const getColorsOption = require('./utils/getColorsOption');
-
     const useColor = getColorsOption(getCompilerConfigArray(this.compiler));
     const protocol = this.options.https ? 'https' : 'http';
     const { address, port } = this.server.address();
@@ -644,20 +645,20 @@ class Server {
     let networkUrlIPv4;
     let networkUrlIPv6;
 
-    if (this.hostname) {
-      if (this.hostname === 'localhost') {
+    if (this.options.host) {
+      if (this.options.host === 'localhost') {
         localhost = prettyPrintUrl('localhost');
       } else {
         let isIP;
 
         try {
-          isIP = ipaddr.parse(this.hostname);
+          isIP = ipaddr.parse(this.options.host);
         } catch (error) {
           // Ignore
         }
 
         if (!isIP) {
-          server = prettyPrintUrl(this.hostname);
+          server = prettyPrintUrl(this.options.host);
         }
       }
     }
@@ -754,36 +755,62 @@ class Server {
     if (this.options.open) {
       const runOpen = require('./utils/runOpen');
 
-      const openTarget = prettyPrintUrl(this.hostname || 'localhost');
+      const openTarget = prettyPrintUrl(this.options.host || 'localhost');
 
       runOpen(openTarget, this.options.open, this.logger);
     }
   }
 
   listen(port, hostname, fn) {
-    if (hostname === 'local-ip') {
-      this.hostname = internalIp.v4.sync() || internalIp.v6.sync() || '0.0.0.0';
-    } else if (hostname === 'local-ipv4') {
-      this.hostname = internalIp.v4.sync() || '0.0.0.0';
-    } else if (hostname === 'local-ipv6') {
-      this.hostname = internalIp.v6.sync() || '::';
-    } else {
-      this.hostname = hostname;
+    if (
+      typeof port !== 'undefined' &&
+      typeof this.options.port !== 'undefined' &&
+      port !== this.options.port
+    ) {
+      this.options.port = port;
+
+      this.logger.warn(
+        'The "port" specified in options is different from the port passed as an argument. Will be used from arguments.'
+      );
+    }
+
+    if (!this.options.port) {
+      this.options.port = port;
     }
 
-    if (typeof port !== 'undefined' && port !== this.options.port) {
+    if (
+      typeof hostname !== 'undefined' &&
+      typeof this.options.host !== 'undefined' &&
+      hostname !== this.options.host
+    ) {
+      this.options.host = hostname;
+
       this.logger.warn(
-        'The port specified in options and the port passed as an argument is different.'
+        'The "host" specified in options is different from the host passed as an argument. Will be used from arguments.'
       );
     }
 
-    return (
-      Server.getFreePort(port || this.options.port)
-        // eslint-disable-next-line no-shadow
-        .then((port) => {
-          this.options.port = port;
+    if (!this.options.host) {
+      this.options.host = hostname;
+    }
+
+    if (this.options.host === 'local-ip') {
+      this.options.host =
+        internalIp.v4.sync() || internalIp.v6.sync() || '0.0.0.0';
+    } else if (this.options.host === 'local-ipv4') {
+      this.options.host = internalIp.v4.sync() || '0.0.0.0';
+    } else if (this.options.host === 'local-ipv6') {
+      this.options.host = internalIp.v6.sync() || '::';
+    }
+
+    return Server.getFreePort(this.options.port)
+      .then((foundPort) => {
+        this.options.port = foundPort;
 
-          return this.server.listen(port, this.hostname, (error) => {
+        return this.server.listen(
+          this.options.port,
+          this.options.host,
+          (error) => {
             if (this.options.hot || this.options.liveReload) {
               this.createSocketServer();
             }
@@ -803,14 +830,14 @@ class Server {
             if (typeof this.options.onListening === 'function') {
               this.options.onListening(this);
             }
-          });
-        })
-        .catch((error) => {
-          if (fn) {
-            fn.call(this.server, error);
           }
-        })
-    );
+        );
+      })
+      .catch((error) => {
+        if (fn) {
+          fn.call(this.server, error);
+        }
+      });
   }
 
   close(cb) {
@@ -874,8 +901,6 @@ class Server {
   }
 
   getStats(statsObj) {
-    const getStatsOption = require('./utils/getStatsOption');
-
     const stats = Server.DEFAULT_STATS;
 
     const configArr = getCompilerConfigArray(this.compiler);
@@ -950,12 +975,12 @@ class Server {
     // these are removed from the hostname in url.parse(),
     // so we have the pure IPv6-address in hostname.
     // always allow localhost host, for convenience (hostname === 'localhost')
-    // allow hostname of listening address  (hostname === this.hostname)
+    // allow hostname of listening address  (hostname === this.options.host)
     const isValidHostname =
       ipaddr.IPv4.isValid(hostname) ||
       ipaddr.IPv6.isValid(hostname) ||
       hostname === 'localhost' ||
-      hostname === this.hostname;
+      hostname === this.options.host;
 
     if (isValidHostname) {
       return true;

test/helpers/test-server.js

@@ -21,19 +21,25 @@ function startFullSetup(config, options, done) {
   server = new Server(options, compiler);
 
   let port;
+
   if (Object.prototype.hasOwnProperty.call(options, 'port')) {
     port = options.port;
   } else {
     console.warn('Using the default port for testing is not recommended');
     port = 8080;
   }
-  const host = Object.prototype.hasOwnProperty.call(options, 'host')
-    ? options.host
-    : 'localhost';
 
-  server.listen(port, host, (err) => {
-    if (err && done) {
-      return done(err);
+  let host;
+
+  if (Object.prototype.hasOwnProperty.call(options, 'host')) {
+    host = options.host;
+  } else {
+    host = 'localhost';
+  }
+
+  server.listen(port, host, (error) => {
+    if (error && done) {
+      return done(error);
     }
 
     if (done) {

test/server/Server.test.js

@@ -16,6 +16,7 @@ jest.mock('sockjs/lib/transport');
 
 const baseDevConfig = {
   port,
+  host: 'localhost',
   static: false,
 };
 
@@ -161,6 +162,130 @@ describe('Server', () => {
     });
   });
 
+  describe('listen', () => {
+    let compiler;
+    let server;
+
+    beforeAll(() => {
+      compiler = webpack(config);
+    });
+
+    it('should work and using "port" and "host" from options', (done) => {
+      const options = {
+        host: 'localhost',
+        port,
+      };
+
+      server = new Server(options, compiler);
+
+      // eslint-disable-next-line no-undefined
+      server.listen(undefined, undefined, () => {
+        const info = server.server.address();
+
+        expect(info.address).toBe('127.0.0.1');
+        expect(info.port).toBe(port);
+
+        server.close(done);
+      });
+    });
+
+    it('should work and using "port" and "host" from arguments', (done) => {
+      server = new Server({}, compiler);
+
+      server.listen(port, '127.0.0.1', () => {
+        const info = server.server.address();
+
+        expect(info.address).toBe('127.0.0.1');
+        expect(info.port).toBe(port);
+
+        server.close(done);
+      });
+    });
+
+    it('should work and using the same "port" and "host" from options and arguments', (done) => {
+      const options = {
+        host: 'localhost',
+        port,
+      };
+
+      server = new Server(options, compiler);
+
+      server.listen(options.port, options.host, () => {
+        const info = server.server.address();
+
+        expect(info.address).toBe('127.0.0.1');
+        expect(info.port).toBe(port);
+
+        server.close(done);
+      });
+    });
+
+    it('should work and using "port" from arguments and "host" from options', (done) => {
+      const options = {
+        host: '127.0.0.1',
+      };
+
+      server = new Server(options, compiler);
+
+      // eslint-disable-next-line no-undefined
+      server.listen(port, undefined, () => {
+        const info = server.server.address();
+
+        expect(info.address).toBe('127.0.0.1');
+        expect(info.port).toBe(port);
+
+        server.close(done);
+      });
+    });
+
+    it('should work and using "port" from options and "port" from arguments', (done) => {
+      const options = {
+        port,
+      };
+
+      server = new Server(options, compiler);
+
+      // eslint-disable-next-line no-undefined
+      server.listen(undefined, '127.0.0.1', () => {
+        const info = server.server.address();
+
+        expect(info.address).toBe('127.0.0.1');
+        expect(info.port).toBe(port);
+
+        server.close(done);
+      });
+    });
+
+    it('should log warning when the "port" and "host" options from options different from arguments', (done) => {
+      const options = {
+        host: '127.0.0.2',
+        port: '9999',
+      };
+
+      server = new Server(compiler, options);
+
+      const loggerWarnSpy = jest.spyOn(server.logger, 'warn');
+
+      server.listen(port, '127.0.0.1', () => {
+        const info = server.server.address();
+
+        expect(loggerWarnSpy).toHaveBeenNthCalledWith(
+          1,
+          'The "port" specified in options is different from the port passed as an argument. Will be used from arguments.'
+        );
+        expect(loggerWarnSpy).toHaveBeenNthCalledWith(
+          2,
+          'The "host" specified in options is different from the host passed as an argument. Will be used from arguments.'
+        );
+        expect(info.address).toBe('127.0.0.1');
+        expect(info.port).toBe(port);
+
+        loggerWarnSpy.mockRestore();
+        server.close(done);
+      });
+    });
+  });
+
   describe('checkHost', () => {
     let compiler;
     let server;