-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow specifying styles as Map
s to guarantee ordering
#200
Conversation
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.
turning into
then they could specify their styles via a
Maybe that's not actually something we should worry about, but I don't know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
package.json
Outdated
@@ -44,6 +44,7 @@ | |||
"babel-core": "^5.8.25", | |||
"babel-loader": "^5.3.2", | |||
"chai": "^3.3.0", | |||
"core-js": "^2.4.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/generate.js
Outdated
let generatedStyles = ""; | ||
|
||
Object.keys(merged).forEach(key => { | ||
merged.forEach((key, val) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(please do so with benchmarks, however, and not just automatically)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/generate.js
Outdated
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
} | ||
|
||
OrderedElements.fromObject = (obj) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should take an optional array argument for keyOrder
so it can be specified explicitly when possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Also, thanks for the fast response on this! 🎆 |
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 |
Yes, but I almost certainly won't get to it until Monday at the earliest |
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.
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.
1d56bc3
to
abf6374
Compare
stringHandlers /* : StringHandlers */, | ||
selectorHandlers /* : SelectorHandler[] */ | ||
) /* */ => { | ||
const result = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:( 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, we can always come back, profile, and optimize the hotspots again. 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have time to do a full review just now. I should be able to return to this soon.
README.md
Outdated
}); | ||
``` | ||
|
||
Note that `Map`s are not fully supported in all browsers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth linking off to some resources here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the mdn link is up above, I'll include a link to the shim here.
stringHandlers /* : StringHandlers */, | ||
selectorHandlers /* : SelectorHandler[] */ | ||
) /* */ => { | ||
const result = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, we can always come back, profile, and optimize the hotspots again. 🚀
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you are returning early on 207, so you can remove the else
here and move return val;
up to the top level.
src/generate.js
Outdated
const sortOrder = {}; | ||
for (let i = 0; i < prefixedRules.length; i++) { | ||
const key = prefixedRules[i][0]; | ||
sortOrder[key] = handledDeclarations.keyOrder.indexOf(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to avoid the array scan in this loop too.
src/generate.js
Outdated
// and we sort them to the top. | ||
prefixedRules.sort((a, b) => { | ||
return sortOrder[a[0]] - sortOrder[b[0]]; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Thanks for catching that. Should be fixed in the newest version.
set(key /* : string */, value /* : any */) { | ||
if (!this.elements.hasOwnProperty(key)) { | ||
this.keyOrder.push(key); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the element is already there, should it move it to the end in keyOrder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. I'm wondering if the before behavior is unexpected and could cause similar bugs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/ordered-elements.js
Outdated
|
||
OrderedElements.from = (obj) => { | ||
if (obj instanceof OrderedElements) { | ||
return obj; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this create a new instance? That would be slower, but probably more correct.
f3d987d
to
0c099bd
Compare
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
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
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
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
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 valueordering.
In order to accomplish this, an
OrderedElements
class was created, which issorta like a
Map
but can only store string keys and lacks most of thefeatures. Internally,
Map
s and objects are converted into this and then theseare merged together to preserve the ordering.
Fixes #199
Test Plan:
npm test
@lencioni @ljharb