Skip to content
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

Preserve style order in prefixProperty #117

Merged
merged 1 commit into from Mar 6, 2017

Conversation

lencioni
Copy link
Contributor

@lencioni lencioni commented Mar 6, 2017

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.
@robinweser
Copy link
Owner

robinweser commented Mar 6, 2017

No worry. It's still faster than 3.0.0 :)
Update coming tomorrow!

@robinweser robinweser merged commit 1e8b248 into robinweser:master Mar 6, 2017
@lencioni lencioni deleted the prefixProperty-order branch March 6, 2017 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants