Repository: ember-cli/ember-cli
Pull request #8096 (2018-10-08 21:44:41)

Use regex to parse /livereload urls
- Using strict `/livereload` matching is not very optimal.
- livereload takes `path` as an option which changes URL for upgrade requests. Ex `locahost:4200/path`. 
- LiveReload protocol insists `path` to end with `/livereload`. http://livereload.com/api/protocol/#basics
- The file `is-live-reload-request.js` can be an independent module like [clean-base-url](https://github.com/ember-cli/clean-base-url) which can be used by middlewares which listen for `upgrade` requests in ember-addons. This check is needed if we have listener for `upgrade` requests as we are now serving livereload and ember app on the same port.

SparshithNR: - [ ]  Use the specified live reload host to avoid fuzzy matching with a regexp (just because the protocol requires it end with /livereload does not mean that every path in the appp ending in /livereload is for the client side live reload usage).
- [x] remove package-lock.json
SparshithNR: This has a hard dependency over the version of `ember-cli-inject-live-reload` [PR](https://github.com/ember-cli/ember-cli-inject-live-reload/pull/55)
rwjblue: > This has a hard dependency over the version of ember-cli-inject-live-reload PR

Can you help me understand this? What specifically makes it a hard dependency?
SparshithNR: We are expecting upgrade requests to be **livereloadPrefix/livereload**. To achieve this livereload.js running in client must get **path** as an argument. [PR](https://github.com/ember-cli/ember-cli-inject-live-reload/pull/55) in `ember-cli-inject-live-reload` does that work and [PR#55](https://github.com/ember-cli/ember-cli-inject-live-reload/pull/55) needs this PR to be included in ember-cli so that **path** being set in livereload is properly interpreted in ember-cli `livereload-server.js`.
rwjblue: Gotcha, thank you for the explanation! I think for now, we need to support both old and new worlds. If we receive a request for `/livereload` we should issue a deprecation instructing folks to upgrade to the new version of ember-cli-inject-live-reload. I _believe_ the new utility file is a great place for this, we could issue the deprecation and continue to allow upgrading...


SparshithNR: Utility file is not accessible from `proxy-server` since it is an addon. We probably do something like `clean-base-url` module. Which imported and used in any project. So that if someone using the same logic to bypass `livereload` requests being consumed by other upgrade handlers, we can `is-live-reload-request` module. One place of truth for everyone so that in future if something changes we can just update this module.
rwjblue: > Utility file is not accessible from proxy-server since it is an addon.

I don't fully understand what you mean. The utility **and** the proxy-server are both part of ember-cli, right? You should be able to require between the two without issue...
SparshithNR: Node is able to resolve this. 
When I run yarn test, I get lint error
I added eslint-disable-next-line, Is it a good practice? 
1) eslint
       should have no errors in lib:
     Error: Code did not pass lint rules
lib/tasks/server/middleware/proxy-server/index.js
  3:37  error  "../../../../utilities/is-live-reload-request" is not published  node/no-unpublished-require
rwjblue: Ya, in this case I think disabling the rule inline is correct.  Those extra `package.json`'s make `node/no-unpublished-require` rule pretty confused (since in this case **all** of those files are actually published together, and these aren't separate packages).

Patch:
diff --git a/lib/tasks/server/livereload-server.js b/lib/tasks/server/livereload-server.js
index b192174d1..24d9603d8 100644
--- a/lib/tasks/server/livereload-server.js
+++ b/lib/tasks/server/livereload-server.js
@@ -8,6 +8,7 @@ const logger = require('heimdalljs-logger')('ember-cli:live-reload:');
 const fs = require('fs');
 const Promise = require('rsvp').Promise;
 const cleanBaseUrl = require('clean-base-url');
+const isLiveReloadRequest = require('../../utilities/is-live-reload-request');
 
 function isNotRemoved(entryTuple) {
   let operation = entryTuple[0];
@@ -102,7 +103,7 @@ module.exports = class LiveReloadServer {
     // Reload on express server restarts
     this.app.on('restart', this.didRestart.bind(this));
     this.httpServer.on('upgrade', (req, socket, head) => {
-      if (req.url.indexOf('/livereload') === 0) {
+      if (isLiveReloadRequest(req.url, this.liveReloadPrefix)) {
         this.liveReloadServer.websocketify(req, socket, head);
       }
     });
diff --git a/lib/tasks/server/middleware/proxy-server/index.js b/lib/tasks/server/middleware/proxy-server/index.js
index cde44f1ba..a4fe1c119 100644
--- a/lib/tasks/server/middleware/proxy-server/index.js
+++ b/lib/tasks/server/middleware/proxy-server/index.js
@@ -1,5 +1,8 @@
 'use strict';
 
+// eslint-disable-next-line
+const isLiveReloadRequest = require('../../../../utilities/is-live-reload-request');
+
 class ProxyServerAddon {
   constructor(project) {
     this.project = project;
@@ -32,7 +35,7 @@ class ProxyServerAddon {
       options.ui.writeLine(`Proxying to ${options.proxy}`);
 
       server.on('upgrade', (req, socket, head) => {
-        this.handleProxiedRequest(req, socket, head, options, proxy);
+        this.handleProxiedRequest({ req, socket, head, options, proxy });
       });
 
       app.use(morgan('dev'));
@@ -40,8 +43,8 @@ class ProxyServerAddon {
     }
   }
 
-  handleProxiedRequest(req, socket, head, options, proxy) {
-    if (req.url.indexOf('/livereload') === -1) {
+  handleProxiedRequest({ req, socket, head, options, proxy }) {
+    if (!isLiveReloadRequest(req.url, options.liveReloadPrefix)) {
       options.ui.writeLine(`Proxying websocket to ${req.url}`);
       proxy.ws(req, socket, head);
     }
diff --git a/lib/utilities/is-live-reload-request.js b/lib/utilities/is-live-reload-request.js
new file mode 100644
index 000000000..08260105e
--- /dev/null
+++ b/lib/utilities/is-live-reload-request.js
@@ -0,0 +1,17 @@
+'use strict';
+
+const cleanBaseUrl = require('clean-base-url');
+const deprecate = require('./deprecate');
+
+module.exports = function isLiveReloadRequest(url, liveReloadPrefix) {
+  let regex = /\/livereload$/gi;
+  if (url === `${cleanBaseUrl(liveReloadPrefix)}livereload`) {
+    return true;
+  } else if (regex.test(url)) {
+    //version needs to be updated according to the this PR (https://github.com/ember-cli/ember-cli-inject-live-reload/pull/55)
+    //in master of ember-cli-inject-live-reload.
+    deprecate(`Upgrade ember-cli-inject-live-reload version to 1.10.0 or above`, true);
+    return true;
+  }
+  return false;
+};


Test patch:
diff --git a/tests/unit/tasks/server/middleware/proxy-server-test.js b/tests/unit/tasks/server/middleware/proxy-server-test.js
index f6a2f2b29..7903967cf 100644
--- a/tests/unit/tasks/server/middleware/proxy-server-test.js
+++ b/tests/unit/tasks/server/middleware/proxy-server-test.js
@@ -13,9 +13,13 @@ describe('proxy-server', function() {
   });
 
   it(`bypass livereload request`, function() {
-    expect(proxyServer.handleProxiedRequest({
-      url: '/livereload',
-    })).to.undefined;
+    let options = {
+      liveReloadPrefix: 'test/',
+    };
+    let req = {
+      url: 'test/livereload',
+    };
+    expect(proxyServer.handleProxiedRequest({ req, options })).to.undefined;
   });
 });
 
diff --git a/tests/unit/utilities/is-live-reload-request-test.js b/tests/unit/utilities/is-live-reload-request-test.js
new file mode 100644
index 000000000..16e1c8659
--- /dev/null
+++ b/tests/unit/utilities/is-live-reload-request-test.js
@@ -0,0 +1,31 @@
+'use strict';
+
+const expect = require('chai').expect;
+const isLiveReloadRequest = require('../../../lib/utilities/is-live-reload-request');
+
+describe('isLiveReloadRequest()', function() {
+  it('/livereload', function() {
+    expect(isLiveReloadRequest('/livereload', '/')).to.be.true;
+  });
+  it('path/livereload', function() {
+    expect(isLiveReloadRequest('path/livereload', 'path/')).to.be.true;
+  });
+  it('path/path/livereload', function() {
+    expect(isLiveReloadRequest('path/path/livereload', 'path/path/')).to.be.true;
+  });
+  it('livereload', function() {
+    expect(isLiveReloadRequest('livereload', '/')).to.be.false;
+  });
+  it('livereload/path', function() {
+    expect(isLiveReloadRequest('livereload/path', '/')).to.be.false;
+  });
+  it('/livereload.js', function() {
+    expect(isLiveReloadRequest('/livereload.js', '/')).to.be.false;
+  });
+  it('path/livereload.js', function() {
+    expect(isLiveReloadRequest('path/livereload.js', 'path/')).to.be.false;
+  });
+  it('path/livereload/path', function() {
+    expect(isLiveReloadRequest('path/livereload/path', 'path/')).to.be.false;
+  });
+});