fix(middleware): simplify stripHost. (#3115)

The stripHost middleware only adds a modified url to the request. That modified url is only used one place. By converting the middleware to a module, the code is simpler and beforeMiddleware modules can reuse karma middleware. (One alternative considered was to move the stripHost in the chain before the beforeMiddleware, but this form seems better). Clean up regex per offline suggestion from zzo@
karma-runner · Sep 6, 2018 · d65e911 · d65e911
1 parent 68b37d3
commit d65e911
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 63 deletions.
diff --git a/lib/middleware/karma.js b/lib/middleware/karma.js
@@ -17,6 +17,7 @@ var url = require('url')
 var _ = require('lodash')
 
 var log = require('../logger').create('middleware:karma')
+var stripHost = require('./strip_host').stripHost
 
 function urlparse (urlStr) {
   var urlObj = url.parse(urlStr, true)
@@ -90,7 +91,11 @@ function createKarmaMiddleware (
     var customClientContextFile = injector.get('config.customClientContextFile')
     var includeCrossOriginAttribute = injector.get('config.crossOriginAttribute')
 
-    var requestUrl = request.normalizedUrl.replace(/\?.*/, '')
+    var normalizedUrl = stripHost(request.url) || request.url
+    // For backwards compatibility in middleware plugins, remove in v4.
+    request.normalizedUrl = normalizedUrl
+
+    var requestUrl = normalizedUrl.replace(/\?.*/, '')
     var requestedRangeHeader = request.headers['range']
 
     // redirect /__karma__ to /__karma__ (trailing slash)

diff --git a/lib/middleware/strip_host.js b/lib/middleware/strip_host.js
@@ -1,18 +1,11 @@
 /**
- * Strip host middleware is responsible for stripping hostname from request path
+ * Strip hostname from request path
  * This to handle requests that uses (normally over proxies) an absoluteURI as request path
  */
 
-function createStripHostMiddleware () {
-  return function (request, response, next) {
-    function stripHostFromUrl (url) {
-      return url.replace(/^http[s]?:\/\/([a-z\-.:\d]+)\//, '/')
-    }
-
-    request.normalizedUrl = stripHostFromUrl(request.url) || request.url
-    next()
-  }
+function stripHostFromUrl (url) {
+  return url.replace(/^https?:\/\/[a-z.:\d-]+\//, '/')
 }
 
 // PUBLIC API
-exports.create = createStripHostMiddleware
+exports.stripHost = stripHostFromUrl
diff --git a/lib/web-server.js b/lib/web-server.js
@@ -10,7 +10,6 @@ const Promise = require('bluebird')
 const common = require('./middleware/common')
 const runnerMiddleware = require('./middleware/runner')
 const stopperMiddleware = require('./middleware/stopper')
-const stripHostMiddleware = require('./middleware/strip_host')
 const karmaMiddleware = require('./middleware/karma')
 const sourceFilesMiddleware = require('./middleware/source_files')
 const proxyMiddleware = require('./middleware/proxy')
@@ -65,7 +64,6 @@ function createWebServer (injector, config) {
 
   handler.use(injector.invoke(runnerMiddleware.create))
   handler.use(injector.invoke(stopperMiddleware.create))
-  handler.use(injector.invoke(stripHostMiddleware.create))
   handler.use(injector.invoke(karmaMiddleware.create))
   handler.use(injector.invoke(sourceFilesMiddleware.create))
   // TODO(vojta): extract the proxy into a plugin

diff --git a/test/unit/middleware/strip_host.spec.js b/test/unit/middleware/strip_host.spec.js
@@ -1,61 +1,28 @@
-var mocks = require('mocks')
-
 describe('middleware.strip_host', function () {
-  var nextSpy
-  var HttpRequestMock = mocks.http.ServerRequest
-
-  var createStripHostMiddleware = require('../../../lib/middleware/strip_host').create
-
-  var handler = nextSpy = null
-
-  beforeEach(function () {
-    nextSpy = sinon.spy()
-    handler = createStripHostMiddleware(null, null, '/base/path')
-    return handler
-  })
+  const stripHost = require('../../../lib/middleware/strip_host').stripHost
 
-  it('should strip request with IP number', function (done) {
-    var request = new HttpRequestMock('http://192.12.31.100/base/a.js?123345')
-    handler(request, null, nextSpy)
-
-    expect(request.normalizedUrl).to.equal('/base/a.js?123345')
-    expect(nextSpy).to.have.been.called
-    return done()
+  it('should strip request with IP number', function () {
+    const normalizedUrl = stripHost('http://192.12.31.100/base/a.js?123345')
+    expect(normalizedUrl).to.equal('/base/a.js?123345')
   })
 
-  it('should strip request with absoluteURI', function (done) {
-    var request = new HttpRequestMock('http://localhost/base/a.js?123345')
-    handler(request, null, nextSpy)
-
-    expect(request.normalizedUrl).to.equal('/base/a.js?123345')
-    expect(nextSpy).to.have.been.called
-    return done()
+  it('should strip request with absoluteURI', function () {
+    const normalizedUrl = stripHost('http://localhost/base/a.js?123345')
+    expect(normalizedUrl).to.equal('/base/a.js?123345')
   })
 
-  it('should strip request with absoluteURI and port', function (done) {
-    var request = new HttpRequestMock('http://localhost:9876/base/a.js?123345')
-    handler(request, null, nextSpy)
-
-    expect(request.normalizedUrl).to.equal('/base/a.js?123345')
-    expect(nextSpy).to.have.been.called
-    return done()
+  it('should strip request with absoluteURI and port', function () {
+    const normalizedUrl = stripHost('http://localhost:9876/base/a.js?123345')
+    expect(normalizedUrl).to.equal('/base/a.js?123345')
   })
 
-  it('should strip request with absoluteURI over HTTPS', function (done) {
-    var request = new HttpRequestMock('https://karma-runner.github.io/base/a.js?123345')
-    handler(request, null, nextSpy)
-
-    expect(request.normalizedUrl).to.equal('/base/a.js?123345')
-    expect(nextSpy).to.have.been.called
-    return done()
+  it('should strip request with absoluteURI over HTTPS', function () {
+    const normalizedUrl = stripHost('https://karma-runner.github.io/base/a.js?123345')
+    expect(normalizedUrl).to.equal('/base/a.js?123345')
   })
 
-  return it('should return same url as passed one', function (done) {
-    var request = new HttpRequestMock('/base/b.js?123345')
-    handler(request, null, nextSpy)
-
-    expect(request.normalizedUrl).to.equal('/base/b.js?123345')
-    expect(nextSpy).to.have.been.called
-    return done()
+  it('should return same url as passed one', function () {
+    const normalizedUrl = stripHost('/base/b.js?123345')
+    expect(normalizedUrl).to.equal('/base/b.js?123345')
   })
 })