search

blakeembrey / free-style

Make CSS easier and more maintainable by using JavaScript
MIT License
707 stars 29 forks source link

add insertion based sorting for overloaded keys #66

Closed dylanmcdiarmid closed 7 years ago

dylanmcdiarmid commented 7 years ago

The current sort is unstable. Using V8 ~6.0, sorting more than 10 style keys results in insertion order for overloads not being respected. The consequences of this, other than rendering overloads out of order, create a particularly insidious bug when server-side rendering. Because the string is different, the hash will be different when we render on a Node server and try to hydrate on something like Firefox, where the sort is still technically unstable, but acts differently than V8. If you are trying to hydrate with something like React, it will complain that the client side generated code doesn't match the server output.

To see an example, add the test to the current branch and watch the output (running Node 8.x, although I don't know how far back it goes). Keep in mind, this isn't just for overrides that go beyond 10 elements, as every other style joins the sort as well. I noticed it using typestyle's csstips module for flexbox which does 3 display overrides, but will render them out of insertion order when you add 8+ extra styles on top of that.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.02%) to 98.214% when pulling 8ec1c4a34425a2c4e9542b024b6a4afa086dab53 on dylanmcdiarmid:master into f71405e570d793e20b7fa9d533f1e88d0c1d99eb on blakeembrey:master.

blakeembrey commented 7 years ago

Just a guess, but why not return 0 when they match instead of keeping extra bits of information around?

blakeembrey commented 7 years ago

Ok, interesting - although it should work (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort) clearly V8 doesn't care. I'll look into it a bit more but this like a reasonable solution.

blakeembrey commented 7 years ago

I copied your test over and went with https://github.com/blakeembrey/free-style/commit/72c98709197aa7a99e1dbfaee45159541935255e to just avoid pushing anything before sorting and handle any arrays afterward. It's little bit more straightforward to follow what's happening, hope it makes sense to you 😄

dylanmcdiarmid commented 7 years ago

LGTM, I think that does keep it simpler. Thanks for the quick update!