diff --git a/README.md b/README.md index e5f969e..65fb5f0 100644 --- a/README.md +++ b/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. diff --git a/package.json b/package.json index 66f543f..6f20e47 100644 --- a/package.json +++ b/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", diff --git a/src/generate.js b/src/generate.js index 6db7a9a..6d0f735 100644 --- a/src/generate.js +++ b/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,55 @@ 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]); + // 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]]; }) ); + // 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 < 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)}`; + sortOrder[`Webkit${capitalizedKey}`] = i - 0.5; + sortOrder[`Moz${capitalizedKey}`] = i - 0.5; + sortOrder[`ms${capitalizedKey}`] = i - 0.5; + } + + // 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 rules = prefixedRules.map(([key, value]) => { const stringValue = stringifyValue(key, value); const ret = `${kebabifyStyleName(key)}:${stringValue};`; diff --git a/src/ordered-elements.js b/src/ordered-elements.js new file mode 100644 index 0000000..f1a72bc --- /dev/null +++ b/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) => OrderedElements; + static from: (Map | {[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); + } +}; diff --git a/src/util.js b/src/util.js index ca5ab72..0f75628 100644 --- a/src/util.js +++ b/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 | any */, + b /* : ObjectMap | Map */ +) /* : 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) } }); diff --git a/tests/generate_test.js b/tests/generate_test.js index 1127f6d..f945539 100644 --- a/tests/generate_test.js +++ b/tests/generate_test.js @@ -1,12 +1,15 @@ import {assert} from 'chai'; +import OrderedElements from '../src/ordered-elements'; import { generateCSSRuleset, generateCSS, defaultSelectorHandlers } from '../src/generate'; describe('generateCSSRuleset', () => { const assertCSSRuleset = (selector, declarations, expected) => { - const actual = generateCSSRuleset(selector, declarations); + const actual = generateCSSRuleset( + selector, + OrderedElements.from(declarations)); assert.equal(actual, expected.split('\n').map(x => x.trim()).join('')); }; it('returns a CSS string for a single property', () => { @@ -153,8 +156,27 @@ describe('generateCSS', () => { it('adds browser prefixes', () => { assertCSS('.foo', [{ display: 'flex', - }], '.foo{display:-moz-box !important;display:-ms-flexbox !important;display:-webkit-box !important;display:-webkit-flex !important;display:flex !important;}', - defaultSelectorHandlers); + transition: 'all 0s', + alignItems: 'center', + justifyContent: 'center', + }], '.foo{' + + '-ms-flex-pack:center !important;' + + '-webkit-box-align:center !important;' + + '-ms-flex-align:center !important;' + + '-webkit-box-pack:center !important;' + + 'display:flex !important;' + + 'display:-webkit-flex !important;' + + 'display:-ms-flexbox !important;' + + 'display:-moz-box !important;' + + 'display:-webkit-box !important;' + + '-webkit-transition:all 0s !important;' + + 'transition:all 0s !important;' + + '-webkit-align-items:center !important;' + + 'align-items:center !important;' + + '-webkit-justify-content:center !important;' + + 'justify-content:center !important;' + + '}', + defaultSelectorHandlers); }); it('supports other selector handlers', () => { diff --git a/tests/ordered-elements_test.js b/tests/ordered-elements_test.js new file mode 100644 index 0000000..93a5392 --- /dev/null +++ b/tests/ordered-elements_test.js @@ -0,0 +1,120 @@ +/* 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); + + assert.deepEqual({ + elements: orig, + keyOrder: ["a", "b"], + }, elems); + }); + + it("generates from a Map", () => { + const orig = new Map([ + ["a", 1], + ["b", 2] + ]); + + const elems = OrderedElements.from(orig); + + assert.deepEqual({ + elements: { + a: 1, + b: 2, + }, + keyOrder: ["a", "b"], + }, elems); + }); + + it("generates from a OrderedElements", () => { + const orig = new OrderedElements(); + + orig.set("a", 1); + orig.set("b", 2); + + const elems = OrderedElements.from(orig); + + assert.deepEqual(orig, elems); + }); + + it("adds new elements in order", () => { + const elems = new OrderedElements(); + + elems.set("a", 1); + elems.set("b", 2); + + assert.deepEqual({ + elements: { + a: 1, + b: 2, + }, + keyOrder: ["a", "b"], + }, elems); + }); + + it("overrides old elements but doesn't add to the key ordering", () => { + const elems = new OrderedElements(); + + elems.set("a", 1); + elems.set("a", 2); + + assert.deepEqual({ + elements: { + a: 2, + }, + keyOrder: ["a"], + }, elems); + }); + + it("iterates over the elements in the correct order", () => { + const elems = new OrderedElements(); + + elems.set("a", 1); + elems.set("b", 2); + elems.set("c", 3); + + const order = []; + + elems.forEach((key, value) => { + order.push([key, value]); + }); + + assert.deepEqual([ + ["a", 1], + ["b", 2], + ["c", 3], + ], order); + }); + + it("maps over the elements, making a new OrderedElements from the result", () => { + const elems = new OrderedElements(); + + elems.set("a", 1); + elems.set("b", 2); + elems.set("c", 3); + + const mapped = elems.map((key, value) => { + return value + 1; + }); + + assert.deepEqual({ + elements: { + a: 2, + b: 3, + c: 4, + }, + keyOrder: ["a", "b", "c"], + }, mapped); + }); +}); diff --git a/tests/util_test.js b/tests/util_test.js index bb826b8..f51bf61 100644 --- a/tests/util_test.js +++ b/tests/util_test.js @@ -1,7 +1,10 @@ +/* global Map */ import {assert} from 'chai'; import {flattenDeep, kebabifyStyleName, recursiveMerge} from '../src/util.js'; +import "es6-shim"; + describe('Utils', () => { describe('flattenDeep', () => { it('flattens arrays at any level', () => { @@ -20,7 +23,10 @@ describe('Utils', () => { a: 2, }), { - a: 2, + elements: { + a: 2, + }, + keyOrder: ["a"], }); assert.deepEqual( @@ -30,8 +36,58 @@ describe('Utils', () => { b: 2, }), { - 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', () => { @@ -42,9 +98,13 @@ describe('Utils', () => { a: [2], }), { - 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({ @@ -53,8 +113,12 @@ describe('Utils', () => { a: null, }), { - a: null, + elements: { + a: null, + }, + keyOrder: ["a"], }); + assert.deepEqual( recursiveMerge({ a: null, @@ -62,7 +126,15 @@ describe('Utils', () => { a: { b: 2 }, }), { - a: { b: 2 }, + elements: { + a: { + elements: { + b: 2, + }, + keyOrder: ["b"], + }, + }, + keyOrder: ["a"], }); }); });