blakeembrey / free-style

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

Custom hash algorithms, detect hash collisions #25

Closed blakeembrey closed 8 years ago

blakeembrey commented 8 years ago

Closes #21.

Edit: If you're implementing your own hash, you'll need to remember to escape (https://github.com/mathiasbynens/cssesc). Don't really feel the need to take on a 150 line dependency for this feature.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.3%) to 99.31% when pulling a2ad111087de1f820d264b549698128c26327595 on configure-hash into 9563a50cd71e40b0918d66d7d75a83f74a20afbf on master.

dmitrage commented 8 years ago

Unfortunately, this change doesn't cover all possible collisions :( Styles with nesting and keyframes are not always catched, because of collision in accumulated hash string.

Gist with failing test cases for the default hash function: https://gist.github.com/dmitrage/1daa3bc5306b176d0df300096441abfb

Keyframes: Identifier for rules is computed from accumulated hash strings, in my case "fromcolor:#0008datocolor:red" and "fromcolor:#000f8ctocolor:red". Default hash function produces same hash value for both: "uybv9". So rules are constructed with rule = "@keyframes huybv9" and styles = "". getIdentifier returns same value, collision not detected.

blakeembrey commented 8 years ago

Great catch, I'm not handling any accumulated hash strings in the detection. Not sure of the best way to do that right now, but I'll think on it.

dmitrage commented 8 years ago

The simplest thing I've found is to add hashString property to Selector and Rule and use it in getIdentifier. Not sure if it is 100% correct but at least all tests are passing. I've forked this branch and created a commit on top to show the idea.

blakeembrey commented 8 years ago

Thanks for the idea, I was hoping not to have to keep around the extra data but it makes sense that I may have to (though the identifier diff is probably going to be a lot more confusing). I'll review and try to get that into the PR for the next release 👍

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.3%) to 99.324% when pulling b09fd3079e9cc9c89a7ba30e2456226381c4099b on configure-hash into fb53fdfc6f8a8f3781ad6fb1c58c746998a48fd3 on master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.3%) to 99.324% when pulling 1daf4df5265c1d6b0de64ae362e259ba2f84c0bd on configure-hash into fb53fdfc6f8a8f3781ad6fb1c58c746998a48fd3 on master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.3%) to 99.324% when pulling bdc9dc67ccfc11b1baf4c67d81ea83f9d00c81c4 on configure-hash into fb53fdfc6f8a8f3781ad6fb1c58c746998a48fd3 on master.