Repository: ViacomInc/data-point (js)

Issue:
Store middleware in a map
**Problem description**:

We store the middleware in an array (`manager.middleware.stack`), and filter that array whenever we need to execute a particular middleware (like `before`, `after`, etc).

https://github.com/ViacomInc/data-point/blob/master/packages/data-point/lib/middleware/resolve.js#L5

It would be more efficient if we don't do this filtering every time we execute a middleware.

**Suggested solution**:

Store the middleware in a map where the keys are a middleware type (`before`, `after`, `before:hash`, etc), and the values are arrays of middleware functions.




Pull request:
refactor(middleware): Store middleware in a map
**What**: more efficient code. This also makes it easy to conditionally execute middleware. For example, we can use code like this in `base-entity#resolve`:
```js
let result = Promise.resolve(accumulator)

if (manager.middleware.store.has('before')) {
  result = result.then(acc => executeMiddleware(manager, accumulator, 'before'))
}
```

(I actually have a follow up branch to make this change: https://github.com/raingerber/data-point/blob/improve-base-entity-resolve/packages/data-point/lib/entity-types/base-entity/resolve.js#L81)

**Why**: closes #231

<!-- How were these changes implemented? -->
**How**: Use a map so that we no longer need the `getStack` function and do not call `middlewareContextFactory.create` if there's no middleware to execute.

<!-- Have you done all of these things?  -->
**Checklist**:
<!-- add "N/A" to the end of each line that's irrelevant to your changes -->
<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->
- [ ] Has Breaking changes
- [ ] Documentation
- [x] Tests
- [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->


Patch:
diff --git a/packages/data-point/lib/middleware-context/factory.js b/packages/data-point/lib/middleware-context/factory.js
index 1de6753..fd03cc1 100644
--- a/packages/data-point/lib/middleware-context/factory.js
+++ b/packages/data-point/lib/middleware-context/factory.js
@@ -1,18 +1,25 @@
-/* eslint no-underscore-dangle: ["error", { "allow": ["___done", "___resolve"] }] */
-
 const _ = require('lodash')
 
+/**
+ * @class
+ * @property {Boolean} ___done
+ * @property {Boolean} ___resolve
+ */
 function MiddlewareContext () {
   this.___done = false
   this.___resolve = false
 }
 
+/**
+ * @param {*} value
+ * @throws if it's been called more than once
+ */
 MiddlewareContext.prototype.resolve = function resolve (value) {
   if (this.___resolve === true || this.___done === true) {
     throw new Error('can not execute resolve() more than once per stack chain.')
   }
 
-  // only assign if argument passed regardless of the value
+  // only assign if an argument is passed, regardless of the value
   if (arguments.length) {
     this.value = value
     this.___resolve = true
@@ -23,6 +30,10 @@ MiddlewareContext.prototype.resolve = function resolve (value) {
 
 module.exports.MiddlewareContext = MiddlewareContext
 
+/**
+ * @param {Object} spec
+ * @return {Object}
+ */
 function create (spec) {
   const middlewareContext = new MiddlewareContext()
   const middlewareContextExtended = _.assignIn(middlewareContext, spec)
diff --git a/packages/data-point/lib/middleware-context/index.js b/packages/data-point/lib/middleware-context/index.js
index 8dd1848..a6a0405 100644
--- a/packages/data-point/lib/middleware-context/index.js
+++ b/packages/data-point/lib/middleware-context/index.js
@@ -1,5 +1,3 @@
-/* eslint global-require: 0 */
-
 module.exports = {
   create: require('./factory').create
 }
diff --git a/packages/data-point/lib/middleware-control/middleware.js b/packages/data-point/lib/middleware-control/middleware.js
index 5a0f161..7141eaf 100644
--- a/packages/data-point/lib/middleware-control/middleware.js
+++ b/packages/data-point/lib/middleware-control/middleware.js
@@ -1,31 +1,36 @@
-/* eslint no-underscore-dangle: ["error", { "allow": ["___done"] }] */
-
 const _ = require('lodash')
 const Promise = require('bluebird')
 
-function run (context, stackSpec, done) {
+/**
+ *
+ * @param {Accumulator} accumulator
+ * @param {Array<Function>} stackSpec
+ * @param {Function} done
+ * @return {*}
+ */
+function run (accumulator, stackSpec, done) {
   if (stackSpec.length === 0) {
-    return done(null, context)
+    return done(null, accumulator)
   }
 
   const stack = stackSpec.slice(0)
 
   function next (err) {
     if (err) {
-      return done(err, context)
+      return done(err, accumulator)
     }
 
-    if (context.___done === true) {
-      return done(null, context)
+    if (accumulator.___done === true) {
+      return done(null, accumulator)
     }
 
     const middlewareFunc = stack.shift()
 
     if (typeof middlewareFunc === 'undefined') {
-      return done(null, context)
+      return done(null, accumulator)
     }
 
-    const execError = _.attempt(middlewareFunc, context, next)
+    const execError = _.attempt(middlewareFunc, accumulator, next)
 
     if (_.isError(execError)) {
       next(execError)
diff --git a/packages/data-point/lib/middleware/factory.js b/packages/data-point/lib/middleware/factory.js
deleted file mode 100644
index 5f21c8c..0000000
--- a/packages/data-point/lib/middleware/factory.js
+++ /dev/null
@@ -1,10 +0,0 @@
-function Middleware (name, callback) {
-  this.name = name
-  this.callback = callback
-}
-
-function create (name, callback) {
-  return new Middleware(name, callback)
-}
-
-module.exports.create = create
diff --git a/packages/data-point/lib/middleware/index.js b/packages/data-point/lib/middleware/index.js
index 8de7fdf..97441b6 100644
--- a/packages/data-point/lib/middleware/index.js
+++ b/packages/data-point/lib/middleware/index.js
@@ -1,6 +1,3 @@
-/* eslint global-require: 0 */
-
 module.exports = {
-  create: require('./factory').create,
   resolve: require('./resolve').resolve
 }
diff --git a/packages/data-point/lib/middleware/resolve.js b/packages/data-point/lib/middleware/resolve.js
index 2d17bfd..52e8eb1 100644
--- a/packages/data-point/lib/middleware/resolve.js
+++ b/packages/data-point/lib/middleware/resolve.js
@@ -1,31 +1,33 @@
+const Promise = require('bluebird')
+
 const middlewareContextFactory = require('../middleware-context')
 const middlewareControl = require('../middleware-control')
 
-function getStack (stack, middlewareName) {
-  return stack.reduce((acc, middleware) => {
-    if (middleware.name === middlewareName) {
-      acc.push(middleware.callback)
-    }
-    return acc
-  }, [])
+/**
+ * @param {Map} store
+ * @param {String} middlewareName
+ * @return {Array}
+ */
+function getStack (store, middlewareName) {
+  return store.get(middlewareName) || []
 }
 
 module.exports.getStack = getStack
 
-function executeStack (stack, middlewareName, accumulator) {
-  const middlewareStack = getStack(stack, middlewareName)
-  return middlewareControl(accumulator, middlewareStack)
-}
-
-module.exports.executeStack = executeStack
-
+/**
+ * @param {Object} manager
+ * @param {String} middlewareName
+ * @param {Accumulator} accumulator
+ * @return {Promise}
+ */
 function resolve (manager, middlewareName, accumulator) {
-  const middlewareContext = middlewareContextFactory.create(accumulator)
-  return executeStack(
-    manager.middleware.stack,
-    middlewareName,
-    middlewareContext
-  )
+  const stack = getStack(manager.middleware.store, middlewareName)
+  if (stack.length === 0) {
+    return Promise.resolve(accumulator)
+  }
+
+  accumulator = middlewareContextFactory.create(accumulator)
+  return middlewareControl(accumulator, stack)
 }
 
 module.exports.resolve = resolve
diff --git a/packages/data-point/lib/stores/middleware.js b/packages/data-point/lib/stores/middleware.js
index 2331ca6..623411d 100644
--- a/packages/data-point/lib/stores/middleware.js
+++ b/packages/data-point/lib/stores/middleware.js
@@ -1,22 +1,36 @@
-const middlewareFactory = require('../middleware')
-
+/**
+ * @param {Object} manager
+ * @return {Object}
+ */
 function clear (manager) {
-  /* eslint no-param-reassign: "off" */
-  manager.stack = []
+  manager.store.clear()
   return manager
 }
+
 module.exports.clear = clear
 
+/**
+ * @param {Object} manager
+ * @param {String} name
+ * @param {Function} callback
+ * @return {Object}
+ */
 function use (manager, name, callback) {
-  manager.stack.push(middlewareFactory.create(name, callback))
-  return manager.stack
+  const stack = manager.store.get(name) || []
+  stack.push(callback)
+  manager.store.set(name, stack)
+  return manager
 }
 
 module.exports.use = use
 
+/**
+ * @param {Object} spec
+ * @return {Object}
+ */
 function create (spec) {
   const manager = {
-    stack: []
+    store: new Map()
   }
 
   manager.use = use.bind(null, manager)


Test patch:
diff --git a/packages/data-point-service/lib/setup-middleware.test.js b/packages/data-point-service/lib/setup-middleware.test.js
index 26b99d4..9e1d27c 100644
--- a/packages/data-point-service/lib/setup-middleware.test.js
+++ b/packages/data-point-service/lib/setup-middleware.test.js
@@ -119,12 +119,20 @@ describe('setupMiddleware', () => {
       settings: {}
     }
     return SetupMiddleware.setupMiddleware(service)
+      .catch(err => err)
       .then(s => {
-        expect(s).toHaveProperty('dataPoint.middleware.stack.length', 2)
-        expect(s).toHaveProperty('dataPoint.middleware.stack.0.name', 'before')
-        expect(s).toHaveProperty('dataPoint.middleware.stack.1.name', 'after')
+        expect(s.dataPoint.middleware.store.size).toBe(2)
+
+        const before = s.dataPoint.middleware.store.get('before')
+        expect(before).toBeInstanceOf(Array)
+        expect(before).toHaveLength(1)
+        expect(before[0]).toBeInstanceOf(Function)
+
+        const after = s.dataPoint.middleware.store.get('after')
+        expect(after).toBeInstanceOf(Array)
+        expect(after).toHaveLength(1)
+        expect(after[0]).toBeInstanceOf(Function)
       })
-      .catch(err => console.log(err))
   })
 
   test('It should use custom before/after if provided', () => {
@@ -136,17 +144,10 @@ describe('setupMiddleware', () => {
       }
     }
     return SetupMiddleware.setupMiddleware(service)
+      .catch(err => err)
       .then(s => {
-        expect(s).toHaveProperty('dataPoint.middleware.stack.length', 2)
-        expect(s).toHaveProperty(
-          'dataPoint.middleware.stack.0.callback',
-          'before'
-        )
-        expect(s).toHaveProperty(
-          'dataPoint.middleware.stack.1.callback',
-          'after'
-        )
+        expect(s.dataPoint.middleware.store.get('before')).toEqual(['before'])
+        expect(s.dataPoint.middleware.store.get('after')).toEqual(['after'])
       })
-      .catch(err => console.log(err))
   })
 })