From abf637437fc9bc3dc60957d3a5a86a68b451b2c1 Mon Sep 17 00:00:00 2001 From: Emily Eisenberg Date: Thu, 2 Mar 2017 16:39:07 -0800 Subject: [PATCH] Allow specifying styles as `Map`s 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 | 74 ++++++++++++++++++++ package.json | 1 + src/generate.js | 73 ++++++++++---------- src/ordered-elements.js | 82 ++++++++++++++++++++++ src/util.js | 31 +++++---- tests/generate_test.js | 15 ++++- tests/ordered-elements_test.js | 120 +++++++++++++++++++++++++++++++++ tests/util_test.js | 84 +++++++++++++++++++++-- 8 files changed, 421 insertions(+), 59 deletions(-) create mode 100644 src/ordered-elements.js create mode 100644 tests/ordered-elements_test.js 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..5512343 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,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};`; diff --git a/src/ordered-elements.js b/src/ordered-elements.js new file mode 100644 index 0000000..abbfec4 --- /dev/null +++ b/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) => 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) { + 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); + } +}; 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..4732d0f 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,14 @@ 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); + }], '.foo{' + + 'display:-webkit-box !important;' + + 'display:-moz-box !important;' + + 'display:-ms-flexbox !important;' + + 'display:-webkit-flex !important;' + + 'display:flex !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"], }); }); });