Refactor out recursiveMerge

We only use this in one place, so I decided to rewrite this for the specific use-case to improve performance. This brings my benchmark down from 1950ms to 1820ms.
Khan · Mar 10, 2017 · 187056b · 187056b
1 parent 12dc739
commit 187056b
Show file tree

Hide file tree

Showing 7 changed files with 26 additions and 239 deletions.
diff --git a/package.json b/package.json
@@ -47,7 +47,6 @@
     "babel-loader": "^5.3.2",
     "caniuse-api": "^1.5.3",
     "chai": "^3.3.0",
-    "es6-shim": "^0.35.3",
     "eslint": "^3.7.1",
     "eslint-config-standard-react": "^4.2.0",
     "eslint-plugin-react": "^6.3.0",

diff --git a/src/generate.js b/src/generate.js
@@ -5,7 +5,6 @@ import staticData from '../lib/staticPrefixData';
 import OrderedElements from './ordered-elements';
 import {
     kebabifyStyleName,
-    recursiveMerge,
     stringifyValue,
     stringifyAndImportantifyValue
 } from './util';
@@ -149,9 +148,13 @@ export const generateCSS = (
     stringHandlers /* : StringHandlers */,
     useImportant /* : boolean */
 ) /* : string */ => {
-    const merged /* : OrderedElements */ = styleTypes.reduce(
-        recursiveMerge,
-        new OrderedElements());
+    const merged = new OrderedElements();
+    for (let i = 0; i < styleTypes.length; i++) {
+        const keys = Object.keys(styleTypes[i]);
+        for (let j = 0; j < keys.length; j++) {
+            merged.set(keys[j], styleTypes[i][keys[j]]);
+        }
+    }
 
     const plainDeclarations = new OrderedElements();
     let generatedStyles = "";

diff --git a/src/ordered-elements.js b/src/ordered-elements.js
@@ -1,5 +1,4 @@
 /* @flow */
-/* global Map */
 
 export default class OrderedElements {
     /* ::
@@ -41,33 +40,3 @@ export default class OrderedElements {
         return this.elements.hasOwnProperty(key);
     }
 }
-
-OrderedElements.fromObject = (obj) => {
-    return new OrderedElements(obj, Object.keys(obj));
-};
-
-OrderedElements.fromMap = (map) => {
-    const ret = new OrderedElements();
-    map.forEach((val, key) => {
-        ret.set(key, val);
-    });
-    return ret;
-};
-
-OrderedElements.from = (obj) => {
-    if (obj instanceof OrderedElements) {
-        // NOTE(emily): This makes a shallow copy of the previous elements, so
-        // if the elements are deeply modified it will affect all copies.
-        return new OrderedElements({...obj.elements}, obj.keyOrder.slice());
-    } else if (
-        // For some reason, flow complains about a plain
-        // `typeof Map !== "undefined"` check. Casting `Map` to `any` solves
-        // the problem.
-        typeof /*::(*/ Map /*: any)*/ !== "undefined" &&
-        obj instanceof Map
-    ) {
-        return OrderedElements.fromMap(obj);
-    } else {
-        return OrderedElements.fromObject(obj);
-    }
-};
diff --git a/src/util.js b/src/util.js
@@ -1,8 +1,6 @@
 /* @flow */
 import stringHash from 'string-hash';
 
-import OrderedElements from './ordered-elements';
-
 /* ::
 type Pair = [ string, any ];
 type Pairs = Pair[];
@@ -37,39 +35,6 @@ export const kebabifyStyleName = (string /* : string */) /* : string */ => {
     return result;
 };
 
-const isPlainObject = (
-  x/* : ObjectMap | any */
-) /* : boolean */ => typeof x === 'object' && !Array.isArray(x) && x !== null;
-
-export const recursiveMerge = (
-    a /* : OrderedElements | ObjectMap | Map<string,any> | any */,
-    b /* : ObjectMap | Map<string,any> */
-) /* : OrderedElements | any */ => {
-    // TODO(jlfwong): Handle malformed input where a and b are not the same
-    // type.
-
-    if (!isPlainObject(a) || !isPlainObject(b)) {
-        if (isPlainObject(b)) {
-            return OrderedElements.from(b);
-        } else {
-            return b;
-        }
-    }
-
-    const ret = OrderedElements.from(a);
-    const right = OrderedElements.from(b);
-
-    right.forEach((key, val) => {
-        if (ret.has(key)) {
-            ret.set(key, recursiveMerge(ret.get(key), val));
-        } else {
-            ret.set(key, val)
-        }
-    });
-
-    return ret;
-};
-
 /**
  * CSS properties which accept numbers but are not in units of "px".
  * Taken from React's CSSProperty.js

diff --git a/tests/generate_test.js b/tests/generate_test.js
@@ -7,9 +7,12 @@ import {
 
 describe('generateCSSRuleset', () => {
     const assertCSSRuleset = (selector, declarations, expected) => {
-        const actual = generateCSSRuleset(
-            selector,
-            OrderedElements.from(declarations));
+        const orderedDeclarations = new OrderedElements();
+        Object.keys(declarations).forEach((key) => {
+            orderedDeclarations.set(key, declarations[key]);
+        });
+
+        const actual = generateCSSRuleset(selector, orderedDeclarations);
         const expectedNormalized = expected.split('\n').map(x => x.trim()).join('');
         const formatStyles = (styles) => styles.replace(/(;|{|})/g, '$1\n');
         assert.equal(

diff --git a/tests/ordered-elements_test.js b/tests/ordered-elements_test.js
@@ -1,51 +1,27 @@
-/* global Map */
 import {assert} from 'chai';
 
 import OrderedElements from '../src/ordered-elements';
 
-import "es6-shim";
-
 describe("OrderedElements", () => {
-    it("generates from an object", () => {
-        const orig = {
-            a: 1,
-            b: 2,
-        };
-
-        const elems = OrderedElements.from(orig);
+    it("can identify elements it has", () => {
+        const elems = new OrderedElements();
 
-        assert.deepEqual({
-            elements: orig,
-            keyOrder: ["a", "b"],
-        }, elems);
+        elems.set("a", 1);
+        assert.equal(elems.has("a"), true);
     });
 
-    it("generates from a Map", () => {
-        const orig = new Map([
-            ["a", 1],
-            ["b", 2]
-        ]);
-
-        const elems = OrderedElements.from(orig);
+    it("can identify elements it does not have", () => {
+        const elems = new OrderedElements();
 
-        assert.deepEqual({
-            elements: {
-                a: 1,
-                b: 2,
-            },
-            keyOrder: ["a", "b"],
-        }, elems);
+        elems.set("a", 1);
+        assert.equal(elems.has("b"), false);
     });
 
-    it("generates from a OrderedElements", () => {
-        const orig = new OrderedElements();
-
-        orig.set("a", 1);
-        orig.set("b", 2);
-
-        const elems = OrderedElements.from(orig);
+    it("can get elements it has", () => {
+        const elems = new OrderedElements();
 
-        assert.deepEqual(orig, elems);
+        elems.set("a", 1);
+        assert.equal(elems.get("a"), 1);
     });
 
     it("adds new elements in order", () => {

diff --git a/tests/util_test.js b/tests/util_test.js
@@ -1,9 +1,6 @@
-/* global Map */
 import {assert} from 'chai';
 
-import {flattenDeep, kebabifyStyleName, recursiveMerge} from '../src/util.js';
-
-import "es6-shim";
+import {flattenDeep, kebabifyStyleName} from '../src/util.js';
 
 describe('Utils', () => {
     describe('flattenDeep', () => {
@@ -14,131 +11,6 @@ describe('Utils', () => {
         });
     });
 
-    describe('recursiveMerge', () => {
-        it('merges two objects', () => {
-            assert.deepEqual(
-                recursiveMerge({
-                    a: 1,
-                }, {
-                    a: 2,
-                }),
-                {
-                    elements: {
-                        a: 2,
-                    },
-                    keyOrder: ["a"],
-                });
-
-            assert.deepEqual(
-                recursiveMerge({
-                    a: 1,
-                }, {
-                    b: 2,
-                }),
-                {
-                    elements: {
-                        a: 1,
-                        b: 2,
-                    },
-                    keyOrder: ["a", "b"],
-                });
-        });
-
-        it('merges maps together', () => {
-            assert.deepEqual(
-                recursiveMerge(
-                    new Map([['a', 1], ['b', 2]]),
-                    new Map([['a', 3], ['c', 4]])
-                ),
-                {
-                    elements: {
-                        a: 3,
-                        b: 2,
-                        c: 4,
-                    },
-                    keyOrder: ["a", "b", "c"],
-                });
-        });
-
-        it('merges maps and objects together', () => {
-            assert.deepEqual(
-                [
-                    new Map([['a', 1]]),
-                    {a: 2, b: 3},
-                    new Map([['b', 4], ['c', 5]]),
-                ].reduce(recursiveMerge),
-                {
-                    elements: {
-                        a: 2,
-                        b: 4,
-                        c: 5,
-                    },
-                    keyOrder: ["a", "b", "c"],
-                });
-        });
-
-        it('generates OrderedElements from merging an object into a non-object', () => {
-            assert.deepEqual(
-                recursiveMerge(
-                    1,
-                    {a: 1},
-                ),
-                {
-                    elements: {
-                        a: 1,
-                    },
-                    keyOrder: ["a"],
-                });
-        });
-        it('replaces arrays rather than merging them', () => {
-            assert.deepEqual(
-                recursiveMerge({
-                    a: [1],
-                }, {
-                    a: [2],
-                }),
-                {
-                    elements: {
-                        a: [2],
-                    },
-                    keyOrder: ["a"],
-                });
-        });
-
-        it('prefers the value from the override object if either property is not a true object', () => {
-            assert.deepEqual(
-                recursiveMerge({
-                    a: { b: 2 },
-                }, {
-                    a: null,
-                }),
-                {
-                    elements: {
-                        a: null,
-                    },
-                    keyOrder: ["a"],
-                });
-
-            assert.deepEqual(
-                recursiveMerge({
-                    a: null,
-                }, {
-                    a: { b: 2 },
-                }),
-                {
-                    elements: {
-                        a: {
-                            elements: {
-                                b: 2,
-                            },
-                            keyOrder: ["b"],
-                        },
-                    },
-                    keyOrder: ["a"],
-                });
-        });
-    });
-
     describe('kebabifyStyleName', () => {
         it('kebabifies camelCase', () => {
             assert.equal(kebabifyStyleName('fooBarBaz'), 'foo-bar-baz');