-
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
Update inline-style-prefixer ^2.0.0 -> ^3.0.1 #205
Conversation
I've been seeing these test failures locally even before this update. I wonder if a change in an earlier version of inline-style-prefixer that wasn't being run in CI caused this. |
I tried v2.0.0 locally and I still get this failure. Perhaps it is spurious in CI? Maybe it is related to the node version or a deep dependency difference? |
I'm trying this out again on a different laptop and I am able to reproduce this locally before and after this change. This looks like it might be a bug in inline-style-prefixer. It looks like inside of { transition: 'border-color 200ms linear',
WebkitTransition: 'border-color 200ms linear',
MozTransition: 'border-color 200ms linear' } which is not the order that we want (we want Also, it seems that @rofrischmann can you take a look at this? My guess at this point is that there something (e.g. Update, I have confirmed that this is a problem with |
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.
Yeah, we do not explicitly order prefixes as we would have to populate the whole object again. We might provide a separate function to do that, but I don't want it to be part of the core as there actually is nothing wrong with the above mentioned order. |
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.
@rofrischmann the order of these is important, since the vendor prefixed versions can do different things. See https://css-tricks.com/ordering-css3-properties/ |
@rofrischmann I think that @xymostech is hoping to get a new release of Aphrodite out today and it would be really great to include this update as well. Any chance you can publish a new version? |
@lencioni @xymostech I've been away yesterday. I can do that today, but later today. Maybe 5pm, its 10.34am right now. (Germany) Edit: Released 🎊 |
Thanks @rofrischmann! 🕺 |
23b5b67
to
3c117be
Compare
Sadly, 3.0.1 didn't actually fix the issue we are running into here. It seems that Edit: ah, it seems that I think we need to look at each plugin and apply the similar ordering fix there. Unfortunately, the plugins are designed in such a way that relies on mutating the style object so this fix is going to require some significant changes. I think it would be best if This is a bit too big of a yak for me to shave right now though, so it would be really great if someone else could dig in. |
Thanks for looking into this @lencioni!! It looks like this isn't going to get out soon enough for the upcoming release, but we can keep track of it for the future. :) |
I think we're not changing that. Immutable objects are just so much slower here. You can easily add another function that sorts the keys once. But I do not want it to be default behavior. Really, I never had any issues with "incorrect" order. Thinks like border-radius e.g. don't get prefixed anyways. |
I wonder if this will perhaps not be an issue after #200 lands. @xymostech I'm happy to try rebasing this once you merge your PR. |
3c117be
to
0a47f10
Compare
It seems that this version is a complete rewrite with some performance improvements. In my benchmarks, this seems to improve the speed of `css()` by about 10%. Changelog: * performance improvements (~10% faster) * ordering prefixed properties correctly * introducing new core prefixer that perform up to 4.5x faster * added a whole new generator to create your custom prefixer * added 4 new plugins to prefix special values * new documentation using gitbook * integrated flowtype It seems that some of the prefixing is different, so I updated the tests to match the new behavior. Since I had a hard time seeing the difference in the test failures, I made them a little easier to see by adding some formatting to the error messages.
0a47f10
to
2b639f6
Compare
@xymostech I've updated this PR on top of your change, and have modified the tests so they pass. It seems that the new version of inline-style-prefixer stops prefixing some things (specifically the old flexbox styles seem to no longer be added, and there are some new moz prefixed styles), so PTAL and pay attention to the changes I've made to the tests. |
You can generate your own prefixer version if you need older browser support ;) |
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.
specifically the old flexbox styles seem to no longer be added
This all LGTM, but I'd hate to break old browsers without people noticing. Do you know how hard it would be to make our own prefixer version like @rofrischmann suggests?
tests/generate_test.js
Outdated
'-webkit-box-pack:center !important;' + | ||
'display:flex !important;' + |
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.
Oh actually now that I look at this more closely, it seems like display: flex
should go after display: -webkit-box
? Maybe we should ensure that. I can do that separately from this change, though.
Other than that, all of these changes seem quite reasonable to me.
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.
Yep, we need display: flex
to be after the webkit one.
'-webkit-transition:border-color 200ms linear !important;' + | ||
'-moz-transition:border-color 200ms linear !important;' + | ||
'transition:border-color 200ms linear !important;' + | ||
'}'); |
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.
<3 formatting
|
||
${formatStyles(actual)} | ||
` | ||
); |
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.
Can I see an example of what an error looks like with this message?
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.
2) generateCSS correctly prefixes border-color transition properties:
AssertionError:
Expected:
.foo{
-webkit-transition:border-color 200ms linear !important;
transition:border-color 200ms linear !important;
}
Actual:
.foo{
-webkit-transition:border-color 200ms linear !important;
-moz-transition:border-color 200ms linear !important;
transition:border-color 200ms linear !important;
}
: expected '.foo{-webkit-transition:border-color 200ms linear !important;-moz-transition:border-color
200ms linear !important;transition:border-color 200ms linear !important;}' to equal '.foo{-webkit-tr
ansition:border-color 200ms linear !important;transition:border-color 200ms linear !important;}'
at assertCSS (tests/generate_test.js:98:8)
at Context.<anonymous> (tests/generate_test.js:204:4)
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.
Awesome.
Looks like maybe there's a guide here? https://github.com/rofrischmann/inline-style-prefixer/blob/master/docs/guides/CustomPrefixAll.md |
@xymostech agreed, it would be best to keep the same browser support. If you update this object to match the versions supported by the current Aphrodite, I'll take a swing at generating our own prefixAll. const browserList = {
chrome: 46,
android: 4,
firefox: 40,
ios_saf: 8,
safari: 8,
ie: 11,
ie_mob: 11,
edge: 12,
opera: 16,
op_mini: 12,
and_uc: 9,
and_chr: 46
} |
Here's a patch that adds the custom prefixAll: https://gist.github.com/xymostech/e01d41c09397b8e4cf4c6b116cc9c9be I mostly just changed |
Oh sweet. It seems like you've got it under control then. Let me know if you'd like me to make any changes to this PR or if you want to merge and follow up with the browser version fixes. |
I think if you just add that patch to this commit that would be good? Now that I think about it, maybe we could just set all of the versions to |
I think that sounds like an okay place to start for now. It would be nice to eventually allow folks to customize this. I'll incorporate your patch shortly. |
@xymostech I just pushed a new commit that includes your patch. I had to make a couple of changes to get it to work. PTAL! |
Looks fine, but I guess now the travis tests are failing. Oops. Maybe add |
The v3 update of inline-style-prefixer changed the set of browsers that it would generate prefixes for by default. To prevent this from being a breaking change for Aphrodite users, we are generating our own prefixer data in a way that supports all versions of all browsers. The guide for this can be found at: https://github.com/rofrischmann/inline-style-prefixer/blob/master/docs/guides/CustomPrefixAll.md It would be nice to eventually allow consumers of Aphrodite to customize the browser support matrix, but in the meantime, this will do.
67eba47
to
54f55a6
Compare
@xymostech done! |
Awesome. I'm gonna take a quick look into moving |
That sounds awesome. I'm really excited to get this into production and see if it has any impact on our performance metrics. |
https://github.com/rofrischmann/inline-style-prefixer/blob/5f456345960512063cd4e3a0f6984fa1ce8951fb/config.js These are the versions, the previous majors 1.x.x and 2.x.x supported. I think you're safe using those. Otherwise you will get a lot of useless prefixes such as |
Oh, thank you @rofrischmann! I wasn't sure where that file lived before. I'll switch to using those versions, since that's actually what we supported before. |
It seems that this version is a complete rewrite with some performance
improvements. In my benchmarks, this seems to improve the speed of
css()
by about 10%.Changelog: