Allow specifying styles as Maps to guarantee ordering

README.md

@@ -429,6 +429,82 @@ 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. It can be
+   polyfilled by using a package
+   like [es6-shim](https://www.npmjs.com/package/es6-shim).
+
 ## 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,65 @@ 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);
+    // Calculate the order that we want to each element in `prefixedRules` to
+    // be in, based on its index in the original key ordering. We have to do
+    // this before the prefixing, because prefixAll mutates
+    // handledDeclarations.elements.
+    const sortOrder = {};
+    for (let i = 0; i < handledDeclarations.keyOrder.length; i++) {
+        const key = handledDeclarations.keyOrder[i];
+        sortOrder[key] = i;
+
+        // In order to keep most prefixed versions of keys in about the same
+        // order that the original keys were in but placed before the
+        // unprefixed version, we generate the prefixed forms of the keys and
+        // set their order to the same as the original key minus a little bit.
+        const capitalizedKey = `${key[0].toUpperCase()}${key.slice(1)}`;
+        const prefixedKeys = [
+            `Webkit${capitalizedKey}`,
+            `Moz${capitalizedKey}`,
+            `ms${capitalizedKey}`,
+        ];
+        for (let j = 0; j < prefixedKeys.length; ++j) {
+            if (!handledDeclarations.elements.hasOwnProperty(prefixedKeys[j])) {
+                sortOrder[prefixedKeys[j]] = i - 0.5;
+            }
+        }
+    }
+
+    // NOTE(emily): This mutates handledDeclarations.elements.
+    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]);
+                // inline-style-prefixer returns an array when there should be
+                // multiple rules for the same key. Here we flatten to multiple
+                // pairs with the same key.
+                return value.map(v => [key, v]);
             }
             return [[key, value]];
         })
     );
 
+    // Sort the prefixed rules according to the order that the keys were
+    // in originally before we prefixed them. New, prefixed versions
+    // of the rules aren't in the original list, so we set their order to -1 so
+    // they sort to the top.
+    prefixedRules.sort((a, b) => {
+        const aOrder = sortOrder.hasOwnProperty(a[0]) ? sortOrder[a[0]] : -1;
+        const bOrder = sortOrder.hasOwnProperty(b[0]) ? sortOrder[b[0]] : -1;
+        return aOrder - bOrder;
+    });
+
     const transformValue = (useImportant === false)
         ? stringifyValue
         : (key, value) => importantify(stringifyValue(key, value));

src/ordered-elements.js

@@ -0,0 +1,84 @@
+/* @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) {
+        // 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);
+    }
+};

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)
         }
     });