Allow specifying styles as Maps to guarantee ordering

Summary: Key ordering in objects can be different in different environments.
Sometimes, this causes problems, like where styles generated on the server are
not in the same order as the same styles generated on the client. This
manifests in problems such as #199

This change lets users manually fix instances where the ordering of elements
changes by specifying their styles in an ES6 `Map`, which has defined value
ordering.

In order to accomplish this, an `OrderedElements` class was created, which is
sorta like a `Map` but can only store string keys and lacks most of the
features. Internally, `Map`s and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:
 - `npm test`

@lencioni @ljharb

README.md

@@ -429,6 +429,80 @@ then we end up with the opposite effect, with `foo` overriding `bar`! The way to
 }
 ```
 
+## Object key ordering
+
+When styles are specified in Aphrodite, the order that they appear in the
+actual stylesheet depends on the order that keys are retrieved from the
+objects. This ordering is determined by the JavaScript engine that is being
+used to render the styles. Sometimes, the order that the styles appear in the
+stylesheet matter for the semantics of the CSS. For instance, depending on the
+engine, the styles generated from
+
+```js
+const styles = StyleSheet.create({
+    ordered: {
+        margin: 0,
+        marginLeft: 15,
+    },
+});
+css(styles.ordered);
+```
+
+you might expect the following CSS to be generated:
+
+```css
+margin: 0px;
+margin-left: 15px;
+```
+
+but depending on the ordering of the keys in the style object, the CSS might
+appear as
+
+```css
+margin-left: 15px;
+margin: 0px;
+```
+
+which is semantically different, because the style which appears later will
+override the style before it.
+
+This might also manifest as a problem when server-side rendering, if the
+generated styles appear in a different order on the client and on the server.
+
+If you experience this issue where styles don't appear in the generated CSS in
+the order that they appear in your objects, there are two solutions:
+
+1. Don't use shorthand properties. For instance, in the margin example above,
+   by switching from using a shorthand property and a longhand property in the
+   same styles to using only longhand properties, the issue could be avoided.
+
+   ```js
+   const styles = StyleSheet.create({
+       ordered: {
+           marginTop: 0,
+           marginRight: 0,
+           marginBottom: 0,
+           marginLeft: 15,
+       },
+   });
+   ```
+
+2. Specify the ordering of your styles by specifying them using a
+   [`Map`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map).
+   Since `Map`s preserve their insertion order, Aphrodite is able to place your
+   styles in the correct order.
+
+   ```js
+   const styles = StyleSheet.create({
+       ordered: new Map([
+           ["margin", 0],
+           ["marginLeft", 15],
+       ]),
+   });
+   ```
+
+   Note that `Map`s are not fully supported in all browsers.
+
 ## Advanced: Extensions
 
 Extra features can be added to Aphrodite using extensions.

package.json

@@ -44,6 +44,7 @@
     "babel-core": "^5.8.25",
     "babel-loader": "^5.3.2",
     "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",

src/generate.js

@@ -1,6 +1,7 @@
 /* @flow */
 import prefixAll from 'inline-style-prefixer/static';
 
+import OrderedElements from './ordered-elements';
 import {
     objectToPairs, kebabifyStyleName, recursiveMerge, stringifyValue,
     importantify, flatten
@@ -143,18 +144,21 @@ export const generateCSS = (
     stringHandlers /* : StringHandlers */,
     useImportant /* : boolean */
 ) /* : string */ => {
-    const merged = styleTypes.reduce(recursiveMerge);
+    const merged /* : OrderedElements */ = styleTypes.reduce(
+        recursiveMerge,
+        new OrderedElements());
 
-    const plainDeclarations = {};
+    const plainDeclarations = new OrderedElements();
     let generatedStyles = "";
 
-    Object.keys(merged).forEach(key => {
+    // TODO(emily): benchmark this to see if a plain for loop would be faster.
+    merged.forEach((key, val) => {
         // For each key, see if one of the selector handlers will handle these
         // styles.
         const foundHandler = selectorHandlers.some(handler => {
             const result = handler(key, selector, (newSelector) => {
                 return generateCSS(
-                    newSelector, [merged[key]], selectorHandlers,
+                    newSelector, [val], selectorHandlers,
                     stringHandlers, useImportant);
             });
             if (result != null) {
@@ -167,7 +171,7 @@ export const generateCSS = (
         // If none of the handlers handled it, add it to the list of plain
         // style declarations.
         if (!foundHandler) {
-            plainDeclarations[key] = merged[key];
+            plainDeclarations.set(key, val);
         }
     });
 
@@ -186,31 +190,25 @@ export const generateCSS = (
  * See generateCSSRuleset for usage and documentation of paramater types.
  */
 const runStringHandlers = (
-    declarations /* : SheetDefinition */,
+    declarations /* : OrderedElements */,
     stringHandlers /* : StringHandlers */,
     selectorHandlers /* : SelectorHandler[] */
 ) /* */ => {
-    const result = {};
-
     const hasStringHandlers = !!stringHandlers;
-    const keys = Object.keys(declarations);
-    for (let i = 0; i < keys.length; i += 1) {
+    return declarations.map((key, val) => {
         // If a handler exists for this particular key, let it interpret
         // that value first before continuing
-        if (hasStringHandlers && stringHandlers.hasOwnProperty(keys[i])) {
+        if (hasStringHandlers && stringHandlers.hasOwnProperty(key)) {
             // TODO(emily): Pass in a callback which generates CSS, similar to
             // how our selector handlers work, instead of passing in
             // `selectorHandlers` and have them make calls to `generateCSS`
             // themselves. Right now, this is impractical because our string
             // handlers are very specialized and do complex things.
-            result[keys[i]] = stringHandlers[keys[i]](
-                declarations[keys[i]], selectorHandlers);
+            return stringHandlers[key](val, selectorHandlers);
         } else {
-            result[keys[i]] = declarations[keys[i]];
+            return val;
         }
-    }
-
-    return result;
+    });
 };
 
 /**
@@ -246,44 +244,43 @@ const runStringHandlers = (
  */
 export const generateCSSRuleset = (
     selector /* : string */,
-    declarations /* : SheetDefinition */,
+    declarations /* : OrderedElements */,
     stringHandlers /* : StringHandlers */,
     useImportant /* : boolean */,
     selectorHandlers /* : SelectorHandler[] */
 ) /* : string */ => {
-    const handledDeclarations = runStringHandlers(
+    const handledDeclarations /* : OrderedElements */ = runStringHandlers(
         declarations, stringHandlers, selectorHandlers);
 
-    const prefixedDeclarations = prefixAll(handledDeclarations);
+    const prefixedDeclarations = prefixAll(handledDeclarations.elements);
 
     const prefixedRules = flatten(
         objectToPairs(prefixedDeclarations).map(([key, value]) => {
             if (Array.isArray(value)) {
                 // inline-style-prefix-all returns an array when there should be
                 // multiple rules, we will flatten to single rules
-
-                const prefixedValues = [];
-                const unprefixedValues = [];
-
-                value.forEach(v => {
-                  if (v[0] === '-') {
-                    prefixedValues.push(v);
-                  } else {
-                    unprefixedValues.push(v);
-                  }
-                });
-
-                prefixedValues.sort();
-                unprefixedValues.sort();
-
-                return prefixedValues
-                  .concat(unprefixedValues)
-                  .map(v => [key, v]);
+                return value.map(v => [key, v]);
             }
             return [[key, value]];
         })
     );
 
+    // Calculate the order that we want to each element in `prefixedRules` to
+    // be in, based on its index in the original key ordering.
+    const sortOrder = {};
+    for (let i = 0; i < prefixedRules.length; i++) {
+        const key = prefixedRules[i][0];
+        sortOrder[key] = handledDeclarations.keyOrder.indexOf(key);
+    }
+
+    // Sort the prefixed rules according to the order that the keys were
+    // in originally before we prefixed them. Since the new, prefixed versions
+    // of the rules aren't in the original list, their sort order will be -1
+    // and we sort them to the top.
+    prefixedRules.sort((a, b) => {
+        return sortOrder[a[0]] - sortOrder[b[0]];
+    });
+
     const rules = prefixedRules.map(([key, value]) => {
         const stringValue = stringifyValue(key, value);
         const ret = `${kebabifyStyleName(key)}:${stringValue};`;

src/ordered-elements.js

@@ -0,0 +1,82 @@
+/* @flow */
+/* global Map */
+
+export default class OrderedElements {
+    /* ::
+    elements: {[string]: any};
+    keyOrder: string[];
+
+    static fromObject: ({[string]: any}) => OrderedElements;
+    static fromMap: (Map<string,any>) => OrderedElements;
+    static from: (Map<string,any> | {[string]: any} | OrderedElements) =>
+        OrderedElements;
+    */
+
+    constructor(
+        elements /* : {[string]: any} */ = {},
+        keyOrder /* : string[] */ = []
+    ) {
+        this.elements = elements;
+        this.keyOrder = keyOrder;
+    }
+
+    forEach(callback /* : (string, any) => void */) {
+        for (let i = 0; i < this.keyOrder.length; i++) {
+            callback(this.keyOrder[i], this.elements[this.keyOrder[i]]);
+        }
+    }
+
+    map(callback /* : (string, any) => any */) /* : OrderedElements */ {
+        const results = new OrderedElements();
+        for (let i = 0; i < this.keyOrder.length; i++) {
+            results.set(
+                this.keyOrder[i],
+                callback(this.keyOrder[i], this.elements[this.keyOrder[i]])
+            );
+        }
+        return results;
+    }
+
+    set(key /* : string */, value /* : any */) {
+        if (!this.elements.hasOwnProperty(key)) {
+            this.keyOrder.push(key);
+        }
+        this.elements[key] = value;
+    }
+
+    get(key /* : string */) /* : any */ {
+        return this.elements[key];
+    }
+
+    has(key /* : string */) /* : boolean */ {
+        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) {
+        return obj;
+    } 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);
+    }
+};

src/util.js

@@ -1,5 +1,7 @@
 /* @flow */
 
+import OrderedElements from './ordered-elements';
+
 /* ::
 type Pair = [ string, any ];
 type Pairs = Pair[];
@@ -41,28 +43,33 @@ export const kebabifyStyleName = (string /* : string */) /* : string */ => {
     return result;
 };
 
-const isNotObject = (
+const isPlainObject = (
   x/* : ObjectMap | any */
-) /* : boolean */ => typeof x !== 'object' || Array.isArray(x) || x === null;
+) /* : boolean */ => typeof x === 'object' && !Array.isArray(x) && x !== null;
 
 export const recursiveMerge = (
-    a /* : ObjectMap | any */,
-    b /* : ObjectMap */
-) /* : ObjectMap */ => {
+    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 (isNotObject(a) || isNotObject(b)) {
-        return b;
+    if (!isPlainObject(a) || !isPlainObject(b)) {
+        if (isPlainObject(b)) {
+            return OrderedElements.from(b);
+        } else {
+            return b;
+        }
     }
 
-    const ret = {...a};
+    const ret = OrderedElements.from(a);
+    const right = OrderedElements.from(b);
 
-    Object.keys(b).forEach(key => {
-        if (ret.hasOwnProperty(key)) {
-            ret[key] = recursiveMerge(a[key], b[key]);
+    right.forEach((key, val) => {
+        if (ret.has(key)) {
+            ret.set(key, recursiveMerge(ret.get(key), val));
         } else {
-            ret[key] = b[key];
+            ret.set(key, val)
         }
     });