sebastiaanvisser / clay

A CSS preprocessor as embedded Haskell.
Other
360 stars 73 forks source link

Usage of floats causes floating errors on output #251

Closed qwbarch closed 1 year ago

qwbarch commented 1 year ago

When using this example css:

"#hello-world" ?
  borderTopLeftRadius (Clay.rem 0.6) (Clay.rem 0.7)

This outputs the following:

#hello-world
{
  border-top-left-radius : 0.59999rem 0.69999rem;
}

Could clay have the input switched to something like Decimal instead, so there's no floating point errors? If a PR is welcome, I'd be happy to make the change!

turion commented 1 year ago

Can confirm. It's also not particular to rem, but also applies to em, cm, pt, and so on. The culprit is probably this function:

cssDoubleText :: Double -> Text
cssDoubleText = fromString . showFixed' . realToFrac
    where
      showFixed' :: Fixed E5 -> String
      showFixed' = showFixed True

I'm guessing that the precision is lost when converting from a Double to a Fixed E5, using realToFrac. In fact I don't understand why Double is used in the first place. We use Fixed E5 to make sure everything has precision of 5 decimal digits, but there is no reason it should first be a Double. Potentially, this is to keep the type signature of rem etc. backwards compatible, but I didn't do the archaeology to check that. Since Fixed has a Num instance, we should be able to specify numeric literals in it directly.

Could clay have the input switched to something like Decimal instead, so there's no floating point errors?

In principle yes, but to avoid additional dependencies, using Fixed E5 directly is better.

If a PR is welcome, I'd be happy to make the change!

Yes, a PR is welcome :) Can you try to change the type signature to

cssDoubleText :: Fixed E5 -> Text

And then solve all the type errors? Maybe give Fixed E5 a type synonym Number, or even a newtype, deriving Num. Also, the name of this helper function should probably be changed to cssNumberText or so.

turion commented 1 year ago

This would then be a breaking change, so a major version bump is required.

qwbarch commented 1 year ago

Yes I'd love to! I'll try and get a PR in within the next couple days once I find some time to work on this

qwbarch commented 1 year ago

@turion Done, should be ready to go: https://github.com/sebastiaanvisser/clay/pull/252