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

lencioni · 2017-03-05T16:39:57Z

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

lencioni · 2017-03-05T18:41:48Z

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.

lencioni · 2017-03-05T18:50:03Z

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?

lencioni · 2017-03-06T19:16:21Z

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 generateCSSRuleset, prefixAll when given { transition: 'border-color 200ms linear' } returns

{ 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 transition to be at the end).

Also, it seems that prefixAll mutates its argument, which I think might be okay for us here but was a bit unexpected to me.

@rofrischmann can you take a look at this? My guess at this point is that there something (e.g. generateStaticPrefixMap) is either ordered incorrectly or there is some code that assumes that Object keys preserve order (similar to #199). However, since I am seeing this in Chrome, I'd probably start by looking for the former.

Update, I have confirmed that this is a problem with prefixProperty. I'll work on a PR to inline-style-prefixer soon.

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 · 2017-03-06T19:53:44Z

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.

lencioni · 2017-03-06T19:56:27Z

@rofrischmann the order of these is important, since the vendor prefixed versions can do different things. See https://css-tricks.com/ordering-css3-properties/

lencioni · 2017-03-07T19:35:57Z

@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?

robinweser · 2017-03-08T09:34:28Z

@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 🎊

lencioni · 2017-03-08T17:15:39Z

Thanks @rofrischmann! 🕺

lencioni · 2017-03-08T17:29:28Z

Sadly, 3.0.1 didn't actually fix the issue we are running into here. It seems that style has already been prefixed by the time it hits prefixProperty. I'll keep looking into this.

Edit: ah, it seems that prefixValue surprisingly mutates the style object in a way that adds the prefixed properties in the wrong order. https://github.com/rofrischmann/inline-style-prefixer/blob/d10d4bb5835a3e797a3355504a7c9f5d2b9d4024/modules/static/createPrefixer.js#L38

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 prefixValue and all of the plugins avoid mutating style and instead return the style object, creating a new one with the correct ordering if there were any changes. @rofrischmann do you think this sounds like the right direction to head in?

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.

xymostech · 2017-03-08T18:54:35Z

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. :)

robinweser · 2017-03-08T18:55:34Z

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.

lencioni · 2017-03-08T20:45:50Z

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.

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.

lencioni · 2017-03-08T21:11:36Z

@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.

robinweser · 2017-03-08T21:16:27Z

You can generate your own prefixer version if you need older browser support ;)

xymostech

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?

xymostech · 2017-03-08T21:14:19Z

tests/generate_test.js

            '-webkit-box-pack:center !important;' +
-            'display:flex !important;' +


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.

Yep, we need display: flex to be after the webkit one.

xymostech · 2017-03-08T21:15:33Z

tests/generate_test.js

+          '-webkit-transition:border-color 200ms linear !important;' +
+          '-moz-transition:border-color 200ms linear !important;' +
+          'transition:border-color 200ms linear !important;' +
+      '}');


<3 formatting

xymostech · 2017-03-08T21:17:18Z

tests/generate_test.js

+
+${formatStyles(actual)}
+`
+        );


Can I see an example of what an error looks like with this message?

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)

xymostech · 2017-03-08T21:22:15Z

Looks like maybe there's a guide here? https://github.com/rofrischmann/inline-style-prefixer/blob/master/docs/guides/CustomPrefixAll.md

lencioni · 2017-03-08T21:37:33Z

@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
}

xymostech · 2017-03-08T21:38:51Z

Here's a patch that adds the custom prefixAll: https://gist.github.com/xymostech/e01d41c09397b8e4cf4c6b116cc9c9be

I mostly just changed ie to 9 though, I haven't really thought through which versions we want to support...

lencioni · 2017-03-08T21:42:10Z

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.

xymostech · 2017-03-08T21:49:18Z

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 0 or 1 or something, since we just sorta want all of the prefixes that are available. Testing that out locally, it seems to do what we want (all the current prefixes plus the old IE ones). Does that sound good? (I updated the patch appropriately)

lencioni · 2017-03-08T21:58:47Z

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.

lencioni · 2017-03-08T22:10:24Z

@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!

xymostech · 2017-03-08T22:16:34Z

Looks fine, but I guess now the travis tests are failing. Oops. Maybe add tools/generate_prefixer_data.js to the pretest script?

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.

lencioni · 2017-03-08T22:18:35Z

@xymostech done!

xymostech · 2017-03-08T22:25:53Z

Awesome. I'm gonna take a quick look into moving display: flex below display: -webkit-flex and then release version 1.2.0. Sound good?

lencioni · 2017-03-08T22:27:09Z

That sounds awesome. I'm really excited to get this into production and see if it has any impact on our performance metrics.

robinweser · 2017-03-08T23:05:51Z

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 borderRadius which really bloats your CSS output.

xymostech · 2017-03-08T23:12:40Z

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.

lencioni mentioned this pull request Mar 6, 2017

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

Merged

lencioni force-pushed the inline-prefixer-3 branch from 23b5b67 to 3c117be Compare March 8, 2017 17:18

lencioni changed the title ~~Update inline-style-prefixer ^2.0.0 -> ^3.0.0~~ Update inline-style-prefixer ^2.0.0 -> ^3.0.1 Mar 8, 2017

lencioni force-pushed the inline-prefixer-3 branch from 3c117be to 0a47f10 Compare March 8, 2017 20:47

lencioni force-pushed the inline-prefixer-3 branch from 0a47f10 to 2b639f6 Compare March 8, 2017 21:10

xymostech approved these changes Mar 8, 2017

View reviewed changes

lencioni force-pushed the inline-prefixer-3 branch from 67eba47 to 54f55a6 Compare March 8, 2017 22:18

xymostech merged commit fb98507 into Khan:master Mar 8, 2017

lencioni deleted the inline-prefixer-3 branch March 8, 2017 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lencioni commented Mar 5, 2017 •

edited

lencioni commented Mar 5, 2017

lencioni commented Mar 5, 2017

lencioni commented Mar 6, 2017 •

edited

robinweser commented Mar 6, 2017

lencioni commented Mar 6, 2017

lencioni commented Mar 7, 2017

robinweser commented Mar 8, 2017 •

edited

lencioni commented Mar 8, 2017

lencioni commented Mar 8, 2017 •

edited

xymostech commented Mar 8, 2017

robinweser commented Mar 8, 2017

lencioni commented Mar 8, 2017

lencioni commented Mar 8, 2017 •

edited

robinweser commented Mar 8, 2017

xymostech left a comment

xymostech Mar 8, 2017

lencioni Mar 8, 2017

xymostech Mar 8, 2017

xymostech Mar 8, 2017

lencioni Mar 8, 2017

xymostech Mar 8, 2017

xymostech commented Mar 8, 2017

lencioni commented Mar 8, 2017

xymostech commented Mar 8, 2017

lencioni commented Mar 8, 2017

xymostech commented Mar 8, 2017

lencioni commented Mar 8, 2017

lencioni commented Mar 8, 2017

xymostech commented Mar 8, 2017

lencioni commented Mar 8, 2017

xymostech commented Mar 8, 2017 •

edited

lencioni commented Mar 8, 2017

robinweser commented Mar 8, 2017

xymostech commented Mar 8, 2017

		'-webkit-box-pack:center !important;' +
		'display:flex !important;' +

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

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

Conversation

lencioni commented Mar 5, 2017 • edited

lencioni commented Mar 5, 2017

lencioni commented Mar 5, 2017

lencioni commented Mar 6, 2017 • edited

robinweser commented Mar 6, 2017

lencioni commented Mar 6, 2017

lencioni commented Mar 7, 2017

robinweser commented Mar 8, 2017 • edited

lencioni commented Mar 8, 2017

lencioni commented Mar 8, 2017 • edited

xymostech commented Mar 8, 2017

robinweser commented Mar 8, 2017

lencioni commented Mar 8, 2017

lencioni commented Mar 8, 2017 • edited

robinweser commented Mar 8, 2017

xymostech left a comment

Choose a reason for hiding this comment

xymostech Mar 8, 2017

Choose a reason for hiding this comment

lencioni Mar 8, 2017

Choose a reason for hiding this comment

xymostech Mar 8, 2017

Choose a reason for hiding this comment

xymostech Mar 8, 2017

Choose a reason for hiding this comment

lencioni Mar 8, 2017

Choose a reason for hiding this comment

xymostech Mar 8, 2017

Choose a reason for hiding this comment

xymostech commented Mar 8, 2017

lencioni commented Mar 8, 2017

xymostech commented Mar 8, 2017

lencioni commented Mar 8, 2017

xymostech commented Mar 8, 2017

lencioni commented Mar 8, 2017

lencioni commented Mar 8, 2017

xymostech commented Mar 8, 2017

lencioni commented Mar 8, 2017

xymostech commented Mar 8, 2017 • edited

lencioni commented Mar 8, 2017

robinweser commented Mar 8, 2017

xymostech commented Mar 8, 2017

lencioni commented Mar 5, 2017 •

edited

lencioni commented Mar 6, 2017 •

edited

robinweser commented Mar 8, 2017 •

edited

lencioni commented Mar 8, 2017 •

edited

lencioni commented Mar 8, 2017 •

edited

xymostech commented Mar 8, 2017 •

edited