Adjust color type to make it more safe

[jfrolich]

The problem

Now that we have PlatformColor (nice implementation btw), I think we should think about color not being a type alias to string anymore (part reversal of this PR). I would like to gradually start to use platform colors in my app, now all colors are strings, and sometimes I do some manipulation on them (like darken/lighten), of course that wouldn't work on platform colors.

Considered solution

So to make colors safe I think we should make them phantom typed.

type t<'a>
external string: string => t<string> = "%identity"
external get: color<'a> => 'a = "%identity"
...
let blue = Color.string("blue")

let blueString = Color.get(blue)

With platform colors being color<PlatformColor.t>.

In Style.t we can accept all colors (we need a lot of type parameters in the external though)

Alternative solution

An opaque type. The type of the color would be lost, so we can't easily have a get or toString that would raise a type error for a platform color. The upside for this is that we need less type parameters.

[MoOx]

Maybe we should "just" have colors (string) and platformColors ? That would definitely make things harder to bind, but maybe not that hard & would let some people to have an easy first approach on color (with strings)?

[jfrolich]

Or a new Style module that is safe and keep the unsafe one around, so it's not a breaking change?

[MoOx]

Just to be sure about the motivation on this change: aren't you supposed to know when you manipulate a color before manipulating it? Or do you have "random" color (string or platformColor) that you put in a registry and you don't know when you are going to lighten or darken one if that's possible or not? With or without types, you are supposed to know upfront anyway right? How is the type system going to help with that, except if you don't really know what you are manipulating ?

I am not closed to anything, but I would like to keep something simple for beginner, and I don't really want to maintain 2 Styles modules called "safe and unsafe" just because of colors. We actually have something way more safe that ReactDOM.Style that accepts string in a lot of place where we accept poly variants or value with unit.

[jfrolich]

I have a big record that contains the color theme of the app (Colors.t), using a hook (Colors.use) I get the light or dark colors for the semantic names. Now all these colors are strings. If I would put in platform colors now they are typed as strings, but actually are not strings, so that is quite error prone. I might assume something is a string, and the type system would not complain.

Worse than that, right now I know I do some manipulation on some colors of the app, if I would convert some colors to platform colors the app would start crashing with type errors in some places.

[jfrolich]

Btw this is very similar to how "margins" are typed currently, where you have to wrap them with dp and pct. So it would be similarly easy/hard for beginners.

[MoOx]

I totally understand your point. Thing is 48.->dp has approximately the same amount of chars than "48px". For "#fff vs "#fff"->Color.fromString or "#fff"->Color.string it's a big longer... (Thinking on my keyboard)...
Maybe we could integrate Color module inside Style, so people can do "#fff"->color ?

About your first proposal, not sure if "Color.get"` has some value, as you cannot get the value of a PlatformColor.

Sure that would be a breaking change anyway as I don't want us to have a "unsafe" Style vs a "safe" Style module (otherwise we could have literally an untyped Style module like ReactDOM.Style vs "what we have + more strict types for color".

I never wrote a codemod, but I think it could be fairly easy to append "->color" to all color style props?

[jfrolich]

I totally understand your point. Thing is 48.->dp has approximately the same amount of chars than "48px". For "#fff vs "#fff"->Color.fromString or "#fff"->Color.string it's a big longer... (Thinking on my keyboard)...
Maybe we could integrate Color module inside Style, so people can do "#fff"->color ?

Yes I think a color function in Style would be nicer indeed.

About your first proposal, not sure if "Color.get"` has some value, as you cannot get the value of a PlatformColor.

It would basically unbox the type, so if you call Color.get on a platformColor you would get PlatformColor.t which would be opaque I assume. The more important thing is that once you replace a color with a platform color, and you use Color.get, it would not type check anymore.

I never wrote a codemod, but I think it could be fairly easy to append "->color" to all color style props?

Yes makes sense to have a codemod for this one, but it might be tricky because if we do a syntax transform we don't know the types, so we don't know if the style function is Style.style

[MoOx]

Ok that's a thing we can do I guess.
Maybe an "approximate" codemod is better than no codemod. Maybe not a lot of people will have style( function similar to ours.
Worst case scenario: people will have to "revert" some changes by the codemod, which is maybe acceptable?
Also: we have style(), viewStyle, textStyle, imageStyle (unsafeStyle doesn't count).