From a599f99adaac012dcf97b67c796d7890b2096bee Mon Sep 17 00:00:00 2001
From: Florian Eitel <feitel@indeedgeek.de>
Date: Thu, 5 Dec 2019 12:01:26 +0100
Subject: [PATCH] refactor(client): createSocketUrl to make it better unit
 tested (#2336)

---
 client-src/default/utils/createSocketUrl.js |  77 ++++++++------
 test/client/utils/createSocketUrl.test.js   | 111 ++++++++++++++++++++
 2 files changed, 154 insertions(+), 34 deletions(-)

diff --git a/client-src/default/utils/createSocketUrl.js b/client-src/default/utils/createSocketUrl.js
index b14e6136cb..d85f546159 100644
--- a/client-src/default/utils/createSocketUrl.js
+++ b/client-src/default/utils/createSocketUrl.js
@@ -3,41 +3,60 @@
 /* global self */
 
 const url = require('url');
-const querystring = require('querystring');
 const getCurrentScriptSource = require('./getCurrentScriptSource');
 
-function createSocketUrl(resourceQuery) {
+function createSocketUrl(resourceQuery, currentLocation) {
   let urlParts;
 
   if (typeof resourceQuery === 'string' && resourceQuery !== '') {
     // If this bundle is inlined, use the resource query to get the correct url.
-    // strip leading `?` from query string
-    urlParts = url.parse(resourceQuery.substr(1));
+    // format is like `?http://0.0.0.0:8096&sockPort=8097&sockHost=localhost`
+    urlParts = url.parse(
+      resourceQuery
+        // strip leading `?` from query string to get a valid URL
+        .substr(1)
+        // replace first `&` with `?` to have a valid query string
+        .replace('&', '?'),
+      true
+    );
   } else {
     // Else, get the url from the <script> this file was called with.
     const scriptHost = getCurrentScriptSource();
-    urlParts = url.parse(scriptHost || '/', false, true);
+    urlParts = url.parse(scriptHost || '/', true, true);
   }
 
-  if (!urlParts.port || urlParts.port === '0') {
-    urlParts.port = self.location.port;
+  // Use parameter to allow passing location in unit tests
+  if (typeof currentLocation === 'string' && currentLocation !== '') {
+    currentLocation = url.parse(currentLocation);
+  } else {
+    currentLocation = self.location;
   }
 
-  const { auth, path } = urlParts;
-  let { hostname, protocol } = urlParts;
+  return getSocketUrl(urlParts, currentLocation);
+}
+
+/*
+ * Gets socket URL based on Script Source/Location
+ * (scriptSrc: URL, location: URL) -> URL
+ */
+function getSocketUrl(urlParts, loc) {
+  const { auth, query } = urlParts;
+  let { hostname, protocol, port } = urlParts;
+
+  if (!port || port === '0') {
+    port = loc.port;
+  }
 
   // check ipv4 and ipv6 `all hostname`
   // why do we need this check?
   // hostname n/a for file protocol (example, when using electron, ionic)
   // see: https://github.com/webpack/webpack-dev-server/pull/384
-  const isAnyHostname =
+  if (
     (hostname === '0.0.0.0' || hostname === '::') &&
-    self.location.hostname &&
-    // eslint-disable-next-line no-bitwise
-    !!~self.location.protocol.indexOf('http');
-
-  if (isAnyHostname) {
-    hostname = self.location.hostname;
+    loc.hostname &&
+    loc.protocol.startsWith('http')
+  ) {
+    hostname = loc.hostname;
   }
 
   // `hostname` can be empty when the script path is relative. In that case, specifying
@@ -47,27 +66,17 @@ function createSocketUrl(resourceQuery) {
   if (
     hostname &&
     hostname !== '127.0.0.1' &&
-    (self.location.protocol === 'https:' || urlParts.hostname === '0.0.0.0')
+    (loc.protocol === 'https:' || urlParts.hostname === '0.0.0.0')
   ) {
-    protocol = self.location.protocol;
+    protocol = loc.protocol;
   }
 
-  // default values of the sock url if they are not provided
-  let sockHost = hostname;
-  let sockPath = '/sockjs-node';
-  let sockPort = urlParts.port;
-
-  // eslint-disable-next-line no-undefined
-  const shouldParsePath = path !== null && path !== undefined && path !== '/';
-  if (shouldParsePath) {
-    const parsedQuery = querystring.parse(path);
-    // all of these sock url params are optionally passed in through
-    // resourceQuery, so we need to fall back to the default if
-    // they are not provided
-    sockHost = parsedQuery.sockHost || sockHost;
-    sockPath = parsedQuery.sockPath || sockPath;
-    sockPort = parsedQuery.sockPort || sockPort;
-  }
+  // all of these sock url params are optionally passed in through
+  // resourceQuery, so we need to fall back to the default if
+  // they are not provided
+  const sockHost = query.sockHost || hostname;
+  const sockPath = query.sockPath || '/sockjs-node';
+  const sockPort = query.sockPort || port;
 
   return url.format({
     protocol,
diff --git a/test/client/utils/createSocketUrl.test.js b/test/client/utils/createSocketUrl.test.js
index 32c3ec59a8..a9ebfb8888 100644
--- a/test/client/utils/createSocketUrl.test.js
+++ b/test/client/utils/createSocketUrl.test.js
@@ -38,4 +38,115 @@ describe('createSocketUrl', () => {
     // put here because resetModules mustn't be reset when L20 is finished
     jest.resetModules();
   });
+
+  const samples2 = [
+    // script source,       location,                output socket URL
+    [
+      'http://example.com',
+      'https://something.com',
+      'https://example.com/sockjs-node',
+    ],
+    [
+      'http://127.0.0.1',
+      'https://something.com',
+      'http://127.0.0.1/sockjs-node',
+    ],
+    [
+      'http://0.0.0.0',
+      'https://something.com',
+      'https://something.com/sockjs-node',
+    ],
+    [
+      'http://0.0.0.0',
+      'http://something.com',
+      'http://something.com/sockjs-node',
+    ],
+    [
+      'http://example.com',
+      'http://something.com',
+      'http://example.com/sockjs-node',
+    ],
+    [
+      'https://example.com',
+      'http://something.com',
+      'https://example.com/sockjs-node',
+    ],
+  ];
+
+  samples2.forEach(([scriptSrc, loc, expected]) => {
+    jest.doMock(
+      '../../../client-src/default/utils/getCurrentScriptSource.js',
+      () => () => scriptSrc
+    );
+
+    const createSocketUrl = require('../../../client-src/default/utils/createSocketUrl');
+
+    test(`should return socket ${expected} for script source ${scriptSrc} and location ${loc}`, () => {
+      // eslint-disable-next-line no-undefined
+      expect(createSocketUrl(undefined, loc).toString()).toEqual(expected);
+    });
+
+    jest.resetModules();
+  });
+
+  const samples3 = [
+    // script source,       location,                output socket URL
+    [
+      '?http://example.com',
+      'https://something.com',
+      'https://example.com/sockjs-node',
+    ],
+    [
+      '?http://127.0.0.1',
+      'https://something.com',
+      'http://127.0.0.1/sockjs-node',
+    ],
+    [
+      '?http://0.0.0.0',
+      'https://something.com',
+      'https://something.com/sockjs-node',
+    ],
+    [
+      '?http://0.0.0.0',
+      'http://something.com',
+      'http://something.com/sockjs-node',
+    ],
+    [
+      '?http://example.com',
+      'http://something.com',
+      'http://example.com/sockjs-node',
+    ],
+    [
+      '?https://example.com',
+      'http://something.com',
+      'https://example.com/sockjs-node',
+    ],
+    [
+      '?https://example.com?sockHost=asdf',
+      'http://something.com',
+      'https://asdf/sockjs-node',
+    ],
+    [
+      '?https://example.com?sockPort=34',
+      'http://something.com',
+      'https://example.com:34/sockjs-node',
+    ],
+    [
+      '?https://example.com?sockPath=xxx',
+      'http://something.com',
+      'https://example.com/xxx',
+    ],
+    [
+      '?http://0.0.0.0:8096&sockPort=8097',
+      'http://localhost',
+      'http://localhost:8097/sockjs-node',
+    ],
+  ];
+  samples3.forEach(([scriptSrc, loc, expected]) => {
+    test(`should return socket ${expected} for query ${scriptSrc} and location ${loc}`, () => {
+      const createSocketUrl = require('../../../client-src/default/utils/createSocketUrl');
+
+      expect(createSocketUrl(scriptSrc, loc).toString()).toEqual(expected);
+    });
+  });
 });