ensure we cache markups with attributes properly

Also change markup creation to use an object of attrs rather than an array

fixes #140

src/js/commands/link.js

@@ -9,9 +9,9 @@ export default class LinkCommand extends TextFormatCommand {
     });
   }
 
-  exec(url) {
+  exec(href) {
     this.editor.run(postEditor => {
-      const markup = postEditor.builder.createMarkup('a', ['href', url]);
+      const markup = postEditor.builder.createMarkup('a', {href});
       this.editor.run(postEditor => postEditor.toggleMarkup(markup));
     });
   }

src/js/models/markup.js

@@ -12,10 +12,13 @@ export const VALID_MARKUP_TAGNAMES = [
 
 class Markup {
   /*
-   * @param {attributes} array flat array of key1,value1,key2,value2,...
+   * @param {Object} attributes key-values
    */
-  constructor(tagName, attributes=[]) {
+  constructor(tagName, attributes={}) {
     this.tagName = normalizeTagName(tagName);
+    if (Array.isArray(attributes)) {
+      throw new Error('Must use attributes object param (not array) to Markup');
+    }
     this.attributes = attributes;
     this.type = MARKUP_TYPE;
 
@@ -25,12 +28,11 @@ class Markup {
   }
 
   hasTag(tagName) {
-    tagName = normalizeTagName(tagName);
-    return this.tagName === tagName;
+    return this.tagName === normalizeTagName(tagName);
   }
 
   static isValidElement(element) {
-    let tagName = normalizeTagName(element.tagName);
+    const tagName = normalizeTagName(element.tagName);
     return VALID_MARKUP_TAGNAMES.indexOf(tagName) !== -1;
   }
 }

src/js/models/post-node-builder.js

@@ -7,6 +7,7 @@ import Marker from '../models/marker';
 import Markup from '../models/markup';
 import Card from '../models/card';
 import { normalizeTagName } from '../utils/dom-utils';
+import { objectToSortedKVArray } from '../utils/array-utils';
 import {
   DEFAULT_TAG_NAME as DEFAULT_MARKUP_SECTION_TAG_NAME
 } from '../models/markup-section';
@@ -15,6 +16,19 @@ import {
   DEFAULT_TAG_NAME as DEFAULT_LIST_SECTION_TAG_NAME
 } from '../models/list-section';
 
+function cacheKey(tagName, attributes) {
+  return `${normalizeTagName(tagName)}-${objectToSortedKVArray(attributes).join('-')}`;
+}
+
+function addMarkupToCache(cache, markup) {
+  cache[cacheKey(markup.tagName, markup.attributes)] = markup;
+}
+
+function findMarkupInCache(cache, tagName, attributes) {
+  const key = cacheKey(tagName, attributes);
+  return cache[key];
+}
+
 export default class PostNodeBuilder {
   constructor() {
     this.markupCache = {};
@@ -75,25 +89,19 @@ export default class PostNodeBuilder {
     return marker;
   }
 
-  // Attributes is an array of [key1, value1, key2, value2, ...]
-  createMarkup(tagName, attributes=[]) {
+  /**
+   * @param {Object} attributes {key:value}
+   */
+  createMarkup(tagName, attributes={}) {
     tagName = normalizeTagName(tagName);
 
-    let markup;
-
-    if (attributes.length) {
-      // FIXME: This could also be cached
+    let markup = findMarkupInCache(this.markupCache, tagName, attributes);
+    if (!markup) {
       markup = new Markup(tagName, attributes);
-    } else {
-      markup = this.markupCache[tagName];
-
-      if (!markup) {
-        markup = new Markup(tagName, attributes);
-        this.markupCache[tagName] = markup;
-      }
+      markup.builder = this;
+      addMarkupToCache(this.markupCache, markup);
     }
 
-    markup.builder = this;
     return markup;
   }
 }

src/js/parsers/dom.js

@@ -1,7 +1,7 @@
 import { trim } from 'content-kit-utils';
 import { VALID_MARKUP_SECTION_TAGNAMES } from '../models/markup-section';
 import { VALID_MARKUP_TAGNAMES } from '../models/markup';
-import { normalizeTagName } from '../utils/dom-utils';
+import { getAttributes, normalizeTagName } from '../utils/dom-utils';
 
 const ELEMENT_NODE = 1;
 const TEXT_NODE = 3;
@@ -12,48 +12,6 @@ function isEmptyTextNode(node) {
   return node.nodeType === TEXT_NODE && trim(node.textContent) === '';
 }
 
-// FIXME we need sorted attributes for deterministic tests. This is not
-// a particularly elegant method, since it loops at least 3 times.
-function sortAttributes(attributes) {
-  let keyValueAttributes = [];
-  let currentKey;
-  attributes.forEach((keyOrValue, index) => {
-    if (index % 2 === 0) {
-      currentKey = keyOrValue;
-    } else {
-      keyValueAttributes.push({key:currentKey, value:keyOrValue});
-    }
-  });
-  keyValueAttributes.sort((a,b) => {
-    return a.key === b.key ? 0 :
-      a.key > b.key ? 1 : - 1;
-  });
-  let sortedAttributes = [];
-  keyValueAttributes.forEach(({key, value}) => {
-    sortedAttributes.push(key, value);
-  });
-  return sortedAttributes;
-}
-
-/**
- * @return {array} attributes as key1,value1,key2,value2,etc
- */
-function readAttributes(node) {
-  var attributes = [];
-
-  if (node.hasAttributes()) {
-    var i, l;
-    for (i=0,l=node.attributes.length;i<l;i++) {
-      if (ALLOWED_ATTRIBUTES.indexOf(node.attributes[i].name) !== -1) {
-        attributes.push(node.attributes[i].name);
-        attributes.push(node.attributes[i].value);
-      }
-    }
-  }
-
-  return sortAttributes(attributes);
-}
-
 function isValidMarkerElement(element) {
   let tagName = normalizeTagName(element.tagName);
   return VALID_MARKUP_TAGNAMES.indexOf(tagName) !== -1;
@@ -67,7 +25,8 @@ function parseMarkers(section, builder, topNode) {
     switch(currentNode.nodeType) {
     case ELEMENT_NODE:
       if (isValidMarkerElement(currentNode)) {
-        markups.push(builder.createMarkup(currentNode.tagName, readAttributes(currentNode)));
+        const attributes = getAttributes(currentNode, ALLOWED_ATTRIBUTES);
+        markups.push(builder.createMarkup(currentNode.tagName, attributes));
       }
       break;
     case TEXT_NODE:

src/js/parsers/mobiledoc.js

@@ -4,7 +4,7 @@ import {
   MOBILEDOC_LIST_SECTION_TYPE,
   MOBILEDOC_CARD_SECTION_TYPE
 } from '../renderers/mobiledoc';
-import { filter } from "../utils/array-utils";
+import { kvArrayToObject, filter } from "../utils/array-utils";
 
 /*
  * input mobiledoc: [ markers, elements ]
@@ -33,8 +33,9 @@ export default class MobiledocParser {
     return markerTypes.map((markerType) => this.parseMarkerType(markerType));
   }
 
-  parseMarkerType([tagName, attributes]) {
-    return this.builder.createMarkup(tagName, attributes);
+  parseMarkerType([tagName, attributesArray]) {
+    const attributesObject = kvArrayToObject(attributesArray || []);
+    return this.builder.createMarkup(tagName, attributesObject);
   }
 
   parseSections(sections, post) {

src/js/parsers/post.js

@@ -6,7 +6,7 @@ import {
 
 import SectionParser from 'content-kit-editor/parsers/section';
 import { forEach } from 'content-kit-editor/utils/array-utils';
-import { getAttributesArray, walkTextNodes } from '../utils/dom-utils';
+import { getAttributes, walkTextNodes } from '../utils/dom-utils';
 import Markup from 'content-kit-editor/models/markup';
 
 export default class PostParser {
@@ -51,9 +51,8 @@ export default class PostParser {
   // Turn an element node into a markup
   markupFromNode(node) {
     if (Markup.isValidElement(node)) {
-      let tagName = node.tagName;
-      let attributes = getAttributesArray(node);
-
+      const tagName = node.tagName;
+      const attributes = getAttributes(node);
       return this.builder.createMarkup(tagName, attributes);
     }
   }

src/js/parsers/section.js

@@ -93,7 +93,6 @@ export default class SectionParser {
   markupFromElement(element) {
     const tagName = normalizeTagName(element.tagName);
     if (VALID_MARKUP_TAGNAMES.indexOf(tagName) === -1) { return null; }
-
     return this.builder.createMarkup(tagName, getAttributes(element));
   }
 

src/js/renderers/editor-dom.js

@@ -17,11 +17,9 @@ export const SPACE = ' ';
 
 function createElementFromMarkup(doc, markup) {
   var element = doc.createElement(markup.tagName);
-  if (markup.attributes) {
-    for (var i=0, l=markup.attributes.length;i<l;i=i+2) {
-      element.setAttribute(markup.attributes[i], markup.attributes[i+1]);
-    }
-  }
+  Object.keys(markup.attributes).forEach(k => {
+    element.setAttribute(k, markup.attributes[k]);
+  });
   return element;
 }
 

src/js/renderers/mobiledoc.js

@@ -1,4 +1,5 @@
 import {visit, visitArray, compile} from '../utils/compiler';
+import { objectToSortedKVArray } from '../utils/array-utils';
 import {
   POST_TYPE,
   MARKUP_SECTION_TYPE,
@@ -44,7 +45,7 @@ const visitor = {
     visitArray(visitor, node.openedMarkups, opcodes);
   },
   [MARKUP_TYPE](node, opcodes) {
-    opcodes.push(['openMarkup', node.tagName, node.attributes]);
+    opcodes.push(['openMarkup', node.tagName, objectToSortedKVArray(node.attributes)]);
   }
 };
 
@@ -84,21 +85,24 @@ const postOpcodeCompiler = {
     };
   },
   openMarkup(tagName, attributes) {
-    if (!this._seenMarkerTypes) {
-      this._seenMarkerTypes = {};
-    }
-    let index;
-    if (attributes.length) {
-      this.markerTypes.push([tagName, attributes]);
-      index = this.markerTypes.length - 1;
-    } else {
-      index = this._seenMarkerTypes[tagName];
-      if (index === undefined) {
-        this.markerTypes.push([tagName]);
-        this._seenMarkerTypes[tagName] = index = this.markerTypes.length-1;
-      }
-    }
+    const index = this._findOrAddMarkerTypeIndex(tagName, attributes);
     this.markupMarkerIds.push(index);
+  },
+  _findOrAddMarkerTypeIndex(tagName, attributesArray) {
+    if (!this._markerTypeCache) { this._markerTypeCache = {}; }
+    const key = `${tagName}-${attributesArray.join('-')}`;
+
+    let index = this._markerTypeCache[key];
+    if (index === undefined) {
+      let markerType = [tagName];
+      if (attributesArray.length) { markerType.push(attributesArray); }
+      this.markerTypes.push(markerType);
+
+      index =  this.markerTypes.length - 1;
+      this._markerTypeCache[key] = index;
+    }
+
+    return index;
   }
 };
 

src/js/utils/array-utils.js

@@ -69,12 +69,37 @@ function reduce(enumerable, callback, initialValue) {
   return previousValue;
 }
 
+/**
+ * @param {Array} array of key1,value1,key2,value2,...
+ * @return {Object} {key1:value1, key2:value2, ...}
+ */
+function kvArrayToObject(array) {
+  const obj = {};
+  for (let i = 0; i < array.length; i+=2) {
+    let [key, value] = [array[i], array[i+1]];
+    obj[key] = value;
+  }
+  return obj;
+}
+
+function objectToSortedKVArray(obj) {
+  const keys = Object.keys(obj).sort();
+  const result = [];
+  keys.forEach(k => {
+    result.push(k);
+    result.push(obj[k]);
+  });
+  return result;
+}
+
 export {
   detect,
   forEach,
   any,
   filter,
   commonItemLength,
   compact,
-  reduce
+  reduce,
+  objectToSortedKVArray,
+  kvArrayToObject
 };

src/js/utils/dom-utils.js

@@ -84,14 +84,23 @@ function containsNode(parentNode, childNode) {
 /**
  * converts the element's NamedNodeMap of attrs into
  * an object with key-value pairs
- * FIXME should add a whitelist as a second arg
+ * @param {DOMNode} element
+ * @param {Array} whitelist optional, an array of attributes to constrain to.
+ * If not passed (or empty), all attributes are allowed.
+ * @return {Object} key-value pairs
  */
-function getAttributes(element) {
-  let result = {};
+function getAttributes(element, whitelist=[]) {
+  const allowed = attrName => {
+    return whitelist.length === 0 ? true : whitelist.indexOf(attrName) !== -1;
+  };
+  const result = {};
   if (element.hasAttributes()) {
-    let attributes = element.attributes;
-
-    forEach(attributes, ({name,value}) => result[name] = value);
+    const attributes = element.attributes;
+    forEach(attributes, ({name,value}) => {
+      if (allowed(name)) {
+        result[name] = value;
+      }
+    });
   }
   return result;
 }

tests/unit/models/markup-section-test.js

@@ -20,20 +20,6 @@ test('a section can append a marker', (assert) => {
   assert.equal(s1.markers.length, 1);
 });
 
-test('postNodeBuilder#createMarkup returns a singleton object when it has no attributes', (assert) => {
-  let markup = builder.createMarkup('b');
-
-  let others = [
-    builder.createMarkup('B'),
-    builder.createMarkup('B', []),
-    builder.createMarkup('b', [])
-  ];
-
-  others.forEach(other => {
-    assert.ok(markup === other, 'markup is the same object');
-  });
-});
-
 test('#splitMarker splits the marker at the offset', (assert) => {
   const m1 = builder.createMarker('hi ');
   const m2 = builder.createMarker('there!');