Refactor commaList filter logic into common util (#12553)

* [utils] extract _.commaSeparatedList mixin to common utils

* [utils] test current version of parseCommaSeparatedList()

* [utils] remove mutation in parseCommaSeparatedList()

* [utils] extract commaList filter to util

* [utils] add tests for formatListAsProse()

* [utils] refactor formatListAsProse(), add oxford comma

* [commaList] add oxford comma to commaList output

* [utils/prose] remove pointless condition

* address PR feedback

* 💥 the stringification thingy

src/ui/public/filters/__tests__/comma_list.js

@@ -12,12 +12,12 @@ describe('Comma-List filter', function () {
   }));
 
   it('converts a string to a pretty list', function () {
-    expect(commaList('john,jaine,jim', true)).to.be('john, jaine and jim');
-    expect(commaList('john,jaine,jim', false)).to.be('john, jaine or jim');
+    expect(commaList('john,jaine,jim', true)).to.be('john, jaine, and jim');
+    expect(commaList('john,jaine,jim', false)).to.be('john, jaine, or jim');
   });
 
   it('can accept an array too', function () {
-    expect(commaList(['john', 'jaine', 'jim'])).to.be('john, jaine or jim');
+    expect(commaList(['john', 'jaine', 'jim'])).to.be('john, jaine, or jim');
   });
 
   it('handles undefined ok', function () {

src/ui/public/filters/comma_list.js

@@ -1,25 +1,22 @@
-import _ from 'lodash';
 import { uiModules } from 'ui/modules';
 
+import {
+  parseCommaSeparatedList,
+  formatListAsProse,
+} from '../../../utils';
+
 uiModules
 .get('kibana')
 .filter('commaList', function () {
   /**
    * Angular filter that accepts either an array or a comma-seperated string
-   * and outputs either an array, or a comma-seperated string for presentation.
+   * and outputs a comma-seperated string for presentation.
    *
    * @param {String|Array} input - The comma-seperated list or array
    * @param {Boolean} inclusive - Should the list be joined with an "and"?
    * @return {String}
    */
-  return function (input, inclusive) {
-    const list = _.commaSeperatedList(input);
-    if (list.length < 2) {
-      return list.join('');
-    }
-
-    const conj = inclusive ? ' and ' : ' or ';
-    return list.slice(0, -1).join(', ') + conj + _.last(list);
-
+  return function (input, inclusive = false) {
+    return formatListAsProse(parseCommaSeparatedList(input), { inclusive });
   };
 });

src/ui/public/utils/lodash-mixins/string.js

@@ -1,25 +0,0 @@
-export function lodashStringMixin(_) {
-  _.mixin({
-
-    /**
-     * Parse a comma-seperated list into an array
-     * efficiently, or just return if already an array
-     *
-     * @param {string|array} input  - the comma-seperated list
-     * @return {array}
-     */
-    commaSeperatedList: function (input) {
-      if (_.isArray(input)) return input;
-
-      const source = String(input || '').split(',');
-      const list = [];
-      while (source.length) {
-        const item = source.shift().trim();
-        if (item) list.push(item);
-      }
-
-      return list;
-    }
-
-  });
-}

src/utils/index.js

@@ -25,3 +25,8 @@ export {
   createSplitStream,
   createMapStream,
 } from './streams';
+
+export {
+  parseCommaSeparatedList,
+  formatListAsProse,
+} from './strings';

src/utils/strings/__tests__/comma_separated_list.js

@@ -0,0 +1,39 @@
+import expect from 'expect.js';
+
+import { parseCommaSeparatedList } from '../comma_separated_list';
+
+describe('utils parseCommaSeparatedList()', () => {
+  it('supports non-string values', () => {
+    expect(parseCommaSeparatedList(0)).to.eql([]);
+    expect(parseCommaSeparatedList(1)).to.eql(['1']);
+    expect(parseCommaSeparatedList({})).to.eql(['[object Object]']);
+    expect(parseCommaSeparatedList(() => {})).to.eql(['() => {}']);
+    expect(parseCommaSeparatedList((a, b) => b)).to.eql(['(a', 'b) => b']);
+    expect(parseCommaSeparatedList(/foo/)).to.eql(['/foo/']);
+    expect(parseCommaSeparatedList(null)).to.eql([]);
+    expect(parseCommaSeparatedList(undefined)).to.eql([]);
+    expect(parseCommaSeparatedList(false)).to.eql([]);
+    expect(parseCommaSeparatedList(true)).to.eql(['true']);
+  });
+
+  it('returns argument untouched if it is an array', () => {
+    const inputs = [
+      [],
+      [1],
+      ['foo,bar']
+    ];
+    for (const input of inputs) {
+      const json = JSON.stringify(input);
+      expect(parseCommaSeparatedList(input)).to.be(input);
+      expect(json).to.be(JSON.stringify(input));
+    }
+  });
+
+  it('trims whitspace around elements', () => {
+    expect(parseCommaSeparatedList('1 ,    2,    3     ,    4')).to.eql(['1', '2', '3', '4']);
+  });
+
+  it('ignored empty elements between multiple commas', () => {
+    expect(parseCommaSeparatedList('foo , , ,,,,, ,      ,bar')).to.eql(['foo', 'bar']);
+  });
+});

src/utils/strings/__tests__/prose.js

@@ -0,0 +1,46 @@
+import expect from 'expect.js';
+
+import { formatListAsProse } from '../prose';
+
+describe('utils formatListAsProse()', () => {
+  it('throw TypeError for non array arguments', () => {
+    const assertTypeError = error => {
+      expect(error).to.be.a(TypeError);
+    };
+
+    expect(() => formatListAsProse(0)).to.throwError(assertTypeError);
+    expect(() => formatListAsProse(1)).to.throwError(assertTypeError);
+    expect(() => formatListAsProse({})).to.throwError(assertTypeError);
+    expect(() => formatListAsProse(() => {})).to.throwError(assertTypeError);
+    expect(() => formatListAsProse((a, b) => b)).to.throwError(assertTypeError);
+    expect(() => formatListAsProse(/foo/)).to.throwError(assertTypeError);
+    expect(() => formatListAsProse(null)).to.throwError(assertTypeError);
+    expect(() => formatListAsProse(undefined)).to.throwError(assertTypeError);
+    expect(() => formatListAsProse(false)).to.throwError(assertTypeError);
+    expect(() => formatListAsProse(true)).to.throwError(assertTypeError);
+  });
+
+  describe('defaults', () => {
+    it('joins items together with "and" and commas', () => {
+      expect(formatListAsProse([1,2])).to.eql('1 and 2');
+      expect(formatListAsProse([1,2,3])).to.eql('1, 2, and 3');
+      expect(formatListAsProse([4,3,2,1])).to.eql('4, 3, 2, and 1');
+    });
+  });
+
+  describe('inclusive=true', () => {
+    it('joins items together with "and" and commas', () => {
+      expect(formatListAsProse([1,2], { inclusive: true })).to.eql('1 and 2');
+      expect(formatListAsProse([1,2,3], { inclusive: true })).to.eql('1, 2, and 3');
+      expect(formatListAsProse([4,3,2,1], { inclusive: true })).to.eql('4, 3, 2, and 1');
+    });
+  });
+
+  describe('inclusive=false', () => {
+    it('joins items together with "or" and commas', () => {
+      expect(formatListAsProse([1,2], { inclusive: false })).to.eql('1 or 2');
+      expect(formatListAsProse([1,2,3], { inclusive: false })).to.eql('1, 2, or 3');
+      expect(formatListAsProse([4,3,2,1], { inclusive: false })).to.eql('4, 3, 2, or 1');
+    });
+  });
+});

src/utils/strings/comma_separated_list.js

@@ -0,0 +1,10 @@
+export function parseCommaSeparatedList(input) {
+  if (Array.isArray(input)) {
+    return input;
+  }
+
+  return String(input || '')
+    .split(',')
+    .map(word => word.trim())
+    .filter(Boolean);
+}

src/utils/strings/index.js

@@ -0,0 +1,2 @@
+export { parseCommaSeparatedList } from './comma_separated_list';
+export { formatListAsProse } from './prose';

src/utils/strings/prose.js

@@ -0,0 +1,30 @@
+/**
+ *  Converts an array of items into a sentence-ready string.
+ *
+ *  @param {Array<any>} list
+ *  @param {Object} [options={}]
+ *  @property {Boolean} [options.inclusive=true] Creates an inclusive list using "and"
+ *                                               when `true` (default), otherwise uses "or"
+ *  @return {String}
+ */
+export function formatListAsProse(list, options = {}) {
+  const {
+    inclusive = true
+  } = options;
+
+  if (!Array.isArray(list)) {
+    throw new TypeError('formatListAsProse() requires an array');
+  }
+
+  const count = list.length;
+  const conjunction = inclusive ? 'and' : 'or';
+
+  if (count <= 2) {
+    return list.join(` ${conjunction} `);
+  }
+
+  return list
+    .slice(0, -1)
+    .concat(`${conjunction} ${list[count - 1]}`)
+    .join(', ');
+}

webpackShims/lodash.js

@@ -6,14 +6,12 @@
  */
 
 var _ = require('node_modules/lodash/index.js').runInContext();
-var lodashStringMixin = require('ui/utils/lodash-mixins/string').lodashStringMixin;
 var lodashLangMixin = require('ui/utils/lodash-mixins/lang').lodashLangMixin;
 var lodashObjectMixin = require('ui/utils/lodash-mixins/object').lodashObjectMixin;
 var lodashCollectionMixin = require('ui/utils/lodash-mixins/collection').lodashCollectionMixin;
 var lodashFunctionMixin = require('ui/utils/lodash-mixins/function').lodashFunctionMixin;
 var lodashOopMixin = require('ui/utils/lodash-mixins/oop').lodashOopMixin;
 
-lodashStringMixin(_);
 lodashLangMixin(_);
 lodashObjectMixin(_);
 lodashCollectionMixin(_);