Allow specifying styles as `Map`s to guarantee ordering by xymostech · Pull Request #200 · Khan/aphrodite

xymostech · 2017-03-03T01:15:10Z

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, Maps and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:

npm test

@lencioni @ljharb

xymostech · 2017-03-03T01:26:13Z

I haven't written docs/examples for this yet, just wanted to throw this out there to see if this sounds like a reasonable solution to the problem you're seeing in #199.

I don't think your code in particular would have to change in order for this to make your issue work correctly, since you're already implicitly specifying the ordering you want via two different style objects. If someone was having problems with e.g.

StyleSheet.create({
    a: {
        margin: 0,
        marginLeft: 15,
    },
})

turning into

margin-left: 15px;
margin: 0px;

then they could specify their styles via a Map like

StyleSheet.create({
    a: new Map([
        ["margin", 0],
        ["marginLeft", 15],
    ]),
})

Maybe that's not actually something we should worry about, but I don't know.

lencioni

This looks like a good improvement to me and I think it would resolve the specific issue I ran into. I worry about the additional overhead, so it might be worth making some performance related tweaks before shipping but overall I think this makes the behavior of Aphrodite more dependable.

lencioni · 2017-03-03T17:37:29Z

    "babel-core": "^5.8.25",
    "babel-loader": "^5.3.2",
    "chai": "^3.3.0",
+    "core-js": "^2.4.1",


Are you bringing this in for Map? It might be a little bit nicer to bring in something a little more scoped like https://github.com/airbnb/browser-shims or just https://www.npmjs.com/package/es6-shim

core-js's Map is not spec-compliant, since it mutates object keys in order to achieve otherwise-impossible performance. I highly recommend using the es6-shim instead here.

Interesting! This is specifically for the few tests which are using Map, not for the actual library, so I'm not super worried about spec compliance here. I can switch it out, though.

lencioni · 2017-03-03T17:39:22Z

    let generatedStyles = "";

-    Object.keys(merged).forEach(key => {
+    merged.forEach((key, val) => {


unrelated: Although I generally think stuff like forEach is best, regular for loops are faster which might matter in performance-sensitive applications. It might be worth looking into optimizing Aphrodite in this way at some point.

(please do so with benchmarks, however, and not just automatically)

Yeah, that's definitely a good point. I'll put in a TODO to think about this later. We haven't done any serious benchmarking yet, so right now I think there are other places that could benefit more from optimization.

lencioni · 2017-03-03T17:41:18Z

+    // of the rules aren't in the original list, we sort them to the top.
+    prefixedRules.sort((a, b) => {
+        const aIndex = handledDeclarations.keyOrder.indexOf(a);
+        const bIndex = handledDeclarations.keyOrder.indexOf(b);


It would be nice to use something with constant time lookup here to avoid all of the array scans in this loop. A simple object would probably do the trick.

lencioni · 2017-03-03T17:43:38Z

+    }
+}
+
+OrderedElements.fromObject = (obj) => {


I wonder if this should take an optional array argument for keyOrder so it can be specified explicitly when possible.

At that point, you'd basically just be calling the constructor, so I'm not sure I see the usefulness of this. Anyway, we're not using that for now; we can add it if it's necessary.

lencioni · 2017-03-03T17:50:27Z

Also, thanks for the fast response on this! 🎆

xymostech · 2017-03-03T19:57:55Z

Thanks for the review, @lencioni! I'll look into making some of the changes you mentioned.

Not sure if this is possible for you, but would it be feasible for you to pull this change into your codebase to make sure that it actually fixes your problem? I'd hate to ship this only to see the bug still manifesting. :P

lencioni · 2017-03-03T23:03:05Z

Yes, but I almost certainly won't get to it until Monday at the earliest

lencioni · 2017-03-06T18:06:16Z

I've tested this out in my dev environment and it seems to resolve the issue we were running into! @xymostech

While attempting to update Aphrodite to inline-style-prefixer 3.0.0, I ran into an issue with prefixAll putting the prefixes in the wrong order. Specifically, they came after the un-prefixed style, which is not what we want because we want the standard style to have precedence in browsers that have it implemented. Khan/aphrodite#205 After a little bit of digging, I found that this was caused by the way prefixProperty was designed. To ensure that the prefixed styles are injected in the correct spot, we need to build a new object key-by-key and return it instead of mutating the style object that was passed in. This is likely to cause a performance hit if there are multiple styles to be prefixed in the same object. It might be worth making a pass to optimize this so all of the styles can be prefixed in one pass, but I'm going to leave that for another time. Although Object ordering is not guaranteed, it is generally sticky in most browsers so this seems like an improvement but not a total fix. This reliance on Object ordering will likely cause issues (different style order) when used on the server on older versions of Node (e.g. Node 4). There should probably be some effort similar to Khan/aphrodite#200 to ensure that the order is properly preserved throughout this package.

xymostech · 2017-03-07T02:29:00Z

    stringHandlers /* : StringHandlers */,
    selectorHandlers /* : SelectorHandler[] */
 ) /* */ => {
-    const result = {};


:( I undid some of your for-loop optimization here, but putting it back is lot uglier now with the OrderedElements thing. I re-did some of the optimizations inside of the OrderedElements implementation, though.

No worries, we can always come back, profile, and optimize the hotspots again. 🚀

xymostech · 2017-03-07T02:31:36Z

@lencioni @ljharb I think I addressed all of the things you mentioned, and I added some docs now too. Mind taking one more look at it before I land? I think once this and the rest of your diffs are landed @lencioni I'll release a new version.

lencioni

I didn't have time to do a full review just now. I should be able to return to this soon.

lencioni · 2017-03-07T17:41:04Z

+   });
+   ```
+
+   Note that `Map`s are not fully supported in all browsers.


It might be worth linking off to some resources here.

https://github.com/airbnb/browser-shims

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

the mdn link is up above, I'll include a link to the shim here.

lencioni · 2017-03-07T17:45:25Z

    stringHandlers /* : StringHandlers */,
    selectorHandlers /* : SelectorHandler[] */
 ) /* */ => {
-    const result = {};


No worries, we can always come back, profile, and optimize the hotspots again. 🚀

lencioni · 2017-03-07T17:46:39Z

-            result[keys[i]] = stringHandlers[keys[i]](
-                declarations[keys[i]], selectorHandlers);
+            return stringHandlers[key](val, selectorHandlers);
        } else {


nit: you are returning early on 207, so you can remove the else here and move return val; up to the top level.

lencioni · 2017-03-07T17:51:20Z

+    const sortOrder = {};
+    for (let i = 0; i < prefixedRules.length; i++) {
+        const key = prefixedRules[i][0];
+        sortOrder[key] = handledDeclarations.keyOrder.indexOf(key);


It would be nice to avoid the array scan in this loop too.

lencioni · 2017-03-07T17:54:23Z

+    // and we sort them to the top.
+    prefixedRules.sort((a, b) => {
+        return sortOrder[a[0]] - sortOrder[b[0]];
+    });


I'm a little worried about this possibly changing the order of some prefixed rules in a problematic way. e.g. if you have a shorthand style and a longhand style of the same property that both need to be prefixed, will the prefixes preserve order? We might need to handle this a bit more delicately.

That's a good point. I'm not sure how to do this perfectly without changing the format that inline-style-prefixer outputs...

What if we do something like sort all of key, "-webkit-" + key, "-moz-" + key, and "-ms-" + key to the same position in the list? There are probably some prefixes we will miss which we'll put at the top, but that should catch most things?

I think that might be okay as long as we only do this for prefixed styles that do not have a sortOrder. In other words, if I explicitly pass in a prefixed style after its unprefixed style, that order should be maintained.

Good point! Thanks for catching that. Should be fixed in the newest version.

lencioni · 2017-03-07T19:16:11Z

+    set(key /* : string */, value /* : any */) {
+        if (!this.elements.hasOwnProperty(key)) {
+            this.keyOrder.push(key);
+        }


If the element is already there, should it move it to the end in keyOrder?

I don't think so. Or at least, that would be semantically different than what we've had before. That is, merging

{ a: 1, b: 2, }

with

{ a: 3, }

gives

{ a: 3, b: 2, }

now. Moving to the end of the keyOrder would change the merged result to

{ b: 2, a: 3, }

That's right. I'm wondering if the before behavior is unexpected and could cause similar bugs?

In the current way, it's impossible to make new styles appear after the old styles. Though if we switch it, it'll be impossible to make new styles appear before the old styles. I think either way has problems, but this at least stays consistent with what we have now.

lencioni · 2017-03-07T19:17:34Z

+
+OrderedElements.from = (obj) => {
+    if (obj instanceof OrderedElements) {
+        return obj;


Should this create a new instance? That would be slower, but probably more correct.

@lencioni

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

@lencioni

Summary: In #200 the way we started sorting prefixed and unprefixed values differently. This builds on that by making sure that when style values are prefixed, they come before the unprefixed values with the same key. E.g. ```css display: -webkit-flex; // prefixed value comes before display: flex; // unprefixed value ``` @lencioni

@lencioni

Summary: In #200 the way we started sorting prefixed and unprefixed values differently. This builds on that by making sure that when style values are prefixed, they come before the unprefixed values with the same key. E.g. ```css display: -webkit-flex; // prefixed value comes before display: flex; // unprefixed value ``` @lencioni

@lencioni

Summary: In #200 the way we started sorting prefixed and unprefixed values differently. This builds on that by making sure that when style values are prefixed, they come before the unprefixed values with the same key. E.g. ```css display: -webkit-flex; // prefixed value comes before display: flex; // unprefixed value ``` @lencioni

lencioni approved these changes Mar 3, 2017

View reviewed changes

lencioni mentioned this pull request Mar 6, 2017

Preserve style order in prefixProperty robinweser/inline-style-prefixer#117

Merged

xymostech force-pushed the map-styles branch 2 times, most recently from 1d56bc3 to abf6374 Compare March 7, 2017 02:28

xymostech commented Mar 7, 2017

View reviewed changes

lencioni reviewed Mar 7, 2017

View reviewed changes

lencioni approved these changes Mar 7, 2017

View reviewed changes

xymostech force-pushed the map-styles branch 2 times, most recently from f3d987d to 0c099bd Compare March 8, 2017 00:12

xymostech force-pushed the map-styles branch from 0c099bd to 58cc212 Compare March 8, 2017 01:11

lencioni mentioned this pull request Mar 8, 2017

Update inline-style-prefixer ^2.0.0 -> ^3.0.1 #205

Merged

xymostech merged commit 3d4a5bd into master Mar 8, 2017

xymostech deleted the map-styles branch March 8, 2017 21:10

xymostech mentioned this pull request Mar 8, 2017

Sort prefixed values before unprefixed values with the same key #211

Merged

ChristopherBiscardi mentioned this pull request Mar 14, 2017

CSS rule ordering can break mixed usage of shorthand and non shorthand properties threepointone/glamor#226

Closed

Conversation

xymostech commented Mar 3, 2017

Uh oh!

xymostech commented Mar 3, 2017

Uh oh!

lencioni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lencioni commented Mar 3, 2017

Uh oh!

xymostech commented Mar 3, 2017

Uh oh!

lencioni commented Mar 3, 2017

Uh oh!

lencioni commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xymostech commented Mar 7, 2017

Uh oh!

lencioni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lencioni commented Mar 6, 2017 •

edited

Loading