Remove unused utilities and add some inline documentation (#59)

packages/commons/lib/filter-query.js

@@ -1,5 +1,6 @@
 const { _ } = require('./utils');
 
+// Officially supported query parameters ($populate is kind of special)
 const PROPERTIES = ['$sort', '$limit', '$skip', '$select', '$populate'];
 
 function parse (number) {
@@ -8,6 +9,8 @@ function parse (number) {
   }
 }
 
+// Returns the pagination limit and will take into account the
+// default and max pagination settings
 function getLimit (limit, paginate) {
   if (paginate && paginate.default) {
     const lower = typeof limit === 'number' ? limit : paginate.default;
@@ -19,18 +22,25 @@ function getLimit (limit, paginate) {
   return limit;
 }
 
+// Makes sure that $sort order is always converted to an actual number
 function convertSort (sort) {
   if (typeof sort !== 'object') {
     return sort;
   }
 
   const result = {};
 
-  Object.keys(sort).forEach(key => (result[key] = typeof sort[key] === 'object' ? sort[key] : parseInt(sort[key], 10)));
+  Object.keys(sort).forEach(key => {
+    result[key] = typeof sort[key] === 'object'
+      ? sort[key] : parseInt(sort[key], 10);
+  });
 
   return result;
 }
 
+// Converts Feathers special query parameters and pagination settings
+// and returns them separately a `filters` and the rest of the query
+// as `query`
 module.exports = function (query, paginate) {
   let filters = {
     $sort: convertSort(query.$sort),

packages/commons/lib/hooks.js

@@ -1,47 +1,43 @@
 const { each } = require('./utils')._;
 
-function getParams (value) {
-  return value || {};
-}
-
 function convertGetOrRemove (args) {
-  return {
-    id: args[0],
-    params: getParams(args[1])
-  };
+  const [ id, params = {} ] = args;
+
+  return { id, params };
 }
 
 function convertUpdateOrPatch (args) {
-  return {
-    id: args[0],
-    data: args[1],
-    params: getParams(args[2])
-  };
+  const [ id, data, params = {} ] = args;
+
+  return { id, data, params };
 }
 
+// Converters from service method arguments to hook object properties
 exports.converters = {
   find (args) {
-    return {
-      params: getParams(args[0])
-    };
+    const [ params = {} ] = args;
+
+    return { params };
   },
   create (args) {
-    return {
-      data: args[0],
-      params: getParams(args[1])
-    };
+    const [ data, params = {} ] = args;
+
+    return { data, params };
   },
   get: convertGetOrRemove,
   remove: convertGetOrRemove,
   update: convertUpdateOrPatch,
   patch: convertUpdateOrPatch
 };
 
+// Create a hook object for a method with arguments `args`
+// `data` is additional data that will be added
 exports.createHookObject = function createHookObject (method, args, data = {}) {
   const hook = exports.converters[method](args);
 
   return Object.assign(hook, data, {
     method,
+    // A dynamic getter that returns the path of the service
     get path () {
       const { app, service } = data;
 
@@ -55,6 +51,7 @@ exports.createHookObject = function createHookObject (method, args, data = {}) {
   });
 };
 
+// Fallback used by `makeArguments` which usually won't be used
 exports.defaultMakeArguments = function defaultMakeArguments (hook) {
   const result = [];
 
@@ -71,26 +68,27 @@ exports.defaultMakeArguments = function defaultMakeArguments (hook) {
   return result;
 };
 
+// Turns a hook object back into a list of arguments
+// to call a service method with
 exports.makeArguments = function makeArguments (hook) {
-  if (hook.method === 'find') {
-    return [ hook.params ];
-  }
-
-  if (hook.method === 'get' || hook.method === 'remove') {
-    return [ hook.id, hook.params ];
-  }
-
-  if (hook.method === 'update' || hook.method === 'patch') {
-    return [ hook.id, hook.data, hook.params ];
-  }
-
-  if (hook.method === 'create') {
-    return [ hook.data, hook.params ];
+  switch (hook.method) {
+    case 'find':
+      return [ hook.params ];
+    case 'get':
+    case 'remove':
+      return [ hook.id, hook.params ];
+    case 'update':
+    case 'patch':
+      return [ hook.id, hook.data, hook.params ];
+    case 'create':
+      return [ hook.data, hook.params ];
   }
 
   return exports.defaultMakeArguments(hook);
 };
 
+// Converts different hook registration formats into the
+// same internal format
 exports.convertHookData = function convertHookData (obj) {
   var hook = {};
 
@@ -107,12 +105,17 @@ exports.convertHookData = function convertHookData (obj) {
   return hook;
 };
 
+// Duck-checks a given object to be a hook object
+// A valid hook object has `type` and `method`
 exports.isHookObject = function isHookObject (hookObject) {
   return typeof hookObject === 'object' &&
     typeof hookObject.method === 'string' &&
     typeof hookObject.type === 'string';
 };
 
+// Returns all service and application hooks combined
+// for a given method and type `appLast` sets if the hooks
+// from `app` should be added last (or first by default)
 exports.getHooks = function getHooks (app, service, type, method, appLast = false) {
   const appHooks = app.__hooks[type][method] || [];
   const serviceHooks = service.__hooks[type][method] || [];
@@ -128,6 +131,8 @@ exports.getHooks = function getHooks (app, service, type, method, appLast = fals
 exports.processHooks = function processHooks (hooks, initialHookObject) {
   let hookObject = initialHookObject;
   let updateCurrentHook = current => {
+    // Either use the returned hook object or the current
+    // hook object from the chain if the hook returned undefined
     if (current) {
       if (!exports.isHookObject(current)) {
         throw new Error(`${hookObject.type} hook for '${hookObject.method}' method returned invalid hook object`);
@@ -138,6 +143,7 @@ exports.processHooks = function processHooks (hooks, initialHookObject) {
 
     return hookObject;
   };
+  // First step of the hook chain with the initial hook object
   let promise = Promise.resolve(hookObject);
 
   // Go through all hooks and chain them into our promise
@@ -148,7 +154,8 @@ exports.processHooks = function processHooks (hooks, initialHookObject) {
       promise = promise.then(hookObject => {
         return new Promise((resolve, reject) => {
           hook(hookObject, (error, result) =>
-            error ? reject(error) : resolve(result));
+            error ? reject(error) : resolve(result)
+          );
         });
       });
     } else { // function(hook)
@@ -166,6 +173,7 @@ exports.processHooks = function processHooks (hooks, initialHookObject) {
   });
 };
 
+// Add `.hooks` functionality to an object
 exports.enableHooks = function enableHooks (obj, methods, types) {
   if (typeof obj.hooks === 'function') {
     return obj;

packages/commons/lib/utils.js

@@ -1,7 +1,9 @@
+// Removes all leading and trailing slashes from a path
 exports.stripSlashes = function stripSlashes (name) {
   return name.replace(/^(\/*)|(\/*)$/g, '');
 };
 
+// A set of lodash-y utility functions that use ES6
 const _ = exports._ = {
   each (obj, callback) {
     if (obj && typeof obj.forEach === 'function') {
@@ -61,11 +63,15 @@ const _ = exports._ = {
     return result;
   },
 
+  // Recursively merge the source object into the target object
   merge (target, source) {
     if (_.isObject(target) && _.isObject(source)) {
       Object.keys(source).forEach(key => {
         if (_.isObject(source[key])) {
-          if (!target[key]) Object.assign(target, { [key]: {} });
+          if (!target[key]) {
+            Object.assign(target, { [key]: {} });
+          }
+
           _.merge(target[key], source[key]);
         } else {
           Object.assign(target, { [key]: source[key] });
@@ -76,36 +82,9 @@ const _ = exports._ = {
   }
 };
 
-exports.specialFilters = {
-  $in (key, ins) {
-    return current => ins.indexOf(current[key]) !== -1;
-  },
-
-  $nin (key, nins) {
-    return current => nins.indexOf(current[key]) === -1;
-  },
-
-  $lt (key, value) {
-    return current => current[key] < value;
-  },
-
-  $lte (key, value) {
-    return current => current[key] <= value;
-  },
-
-  $gt (key, value) {
-    return current => current[key] > value;
-  },
-
-  $gte (key, value) {
-    return current => current[key] >= value;
-  },
-
-  $ne (key, value) {
-    return current => current[key] !== value;
-  }
-};
-
+// Return a function that filters a result object or array
+// and picks only the fields passed as `params.query.$select`
+// and additional `otherFields`
 exports.select = function select (params, ...otherFields) {
   const fields = params && params.query && params.query.$select;
 
@@ -130,33 +109,8 @@ exports.select = function select (params, ...otherFields) {
   };
 };
 
-exports.matcher = function matcher (originalQuery) {
-  const query = _.omit(originalQuery, '$limit', '$skip', '$sort', '$select');
-
-  return function (item) {
-    if (query.$or && _.some(query.$or, or => matcher(or)(item))) {
-      return true;
-    }
-
-    return _.every(query, (value, key) => {
-      if (value !== null && typeof value === 'object') {
-        return _.every(value, (target, filterType) => {
-          if (exports.specialFilters[filterType]) {
-            const filter = exports.specialFilters[filterType](key, target);
-            return filter(item);
-          }
-
-          return false;
-        });
-      } else if (typeof item[key] !== 'undefined') {
-        return item[key] === query[key];
-      }
-
-      return false;
-    });
-  };
-};
-
+// An in-memory sorting function according to the
+// $sort special query parameter
 exports.sorter = function sorter ($sort) {
   return function (first, second) {
     let comparator = 0;
@@ -175,19 +129,7 @@ exports.sorter = function sorter ($sort) {
   };
 };
 
-exports.makeUrl = function makeUrl (path, app = {}) {
-  const get = typeof app.get === 'function' ? app.get.bind(app) : () => {};
-  const env = get('env') || process.env.NODE_ENV;
-  const host = get('host') || process.env.HOST_NAME || 'localhost';
-  const protocol = (env === 'development' || env === 'test' || (env === undefined)) ? 'http' : 'https';
-  const PORT = get('port') || process.env.PORT || 3030;
-  const port = (env === 'development' || env === 'test' || (env === undefined)) ? `:${PORT}` : '';
-
-  path = path || '';
-
-  return `${protocol}://${host}${port}/${exports.stripSlashes(path)}`;
-};
-
+// Duck-checks if an object looks like a promise
 exports.isPromise = function isPromise (result) {
   return _.isObject(result) &&
     typeof result.then === 'function';

packages/commons/test/hooks.test.js

@@ -308,6 +308,30 @@ describe('hook utilities', () => {
       });
     });
 
+    it('next and next with error', () => {
+      const dummyHook = {
+        type: 'dummy',
+        method: 'something'
+      };
+
+      const promise = utils.processHooks([
+        function (hook, next) {
+          hook.test = 'first ran';
+
+          next();
+        },
+
+        function (hook, next) {
+          next(new Error('This did not work'));
+        }
+      ], dummyHook);
+
+      return promise.catch(error => {
+        expect(error.message).to.equal('This did not work');
+        expect(error.hook.test).to.equal('first ran');
+      });
+    });
+
     it('errors when invalid hook object is returned', () => {
       const dummyHook = {
         type: 'dummy',

packages/commons/test/module.test.js

@@ -7,7 +7,6 @@ describe('module', () => {
     console.log(commons);
     expect(typeof commons).to.equal('object');
     expect(typeof commons.stripSlashes).to.equal('function');
-    expect(typeof commons.matcher).to.equal('function');
     expect(typeof commons.sorter).to.equal('function');
     expect(typeof commons.select).to.equal('function');
     expect(typeof commons.validateArguments).to.equal('function');