search

fable-compiler / fable-react

Fable bindings and helpers for React and React Native
MIT License
275 stars 67 forks source link

TextAlign of obj #141

Closed dbrattli closed 5 years ago

dbrattli commented 5 years ago

I did the mistake of div [ Style [TextAlign Left]] [...] which compiles just fine to nothing (without any error or warning) because of TextAlign of obj doesn't give much hint that it wants a string. Do I use the library wrong since I don't get strong typing? Can't we do better in F#?

MangelMaxime commented 5 years ago

We used obj in the bindings because it was either to create like that. Some properties use string others float, int, bool, etc.

As for all the bindings, we can manually improve the type system to help the user. And in this, we can probably accept a PR in order to change obj to string.

dbrattli commented 5 years ago

Should we change to string or use a DU of keyword values? Using string should not be a breaking change, but I guess a DU will break current programs.

MangelMaxime commented 5 years ago

I think we should use string to not break existing code because it would not make sense to have a really strongly typed case and all the others loose.

If we want a fully strongly typed library for the Props we can create one as standalone I think.

alfonsogarciacaro commented 5 years ago

At the beginning we had erased unions in most cases but it was discussed somewhere to change them to obj because it was cumbersome to use !^ everywhere and sometimes the signature was not even right.

Using a string it's probably less invasive although having a StringEnum (with RequireQualifiedAccess) would be nice, and we'll be pushing a new major version for the hooks, so the breaking change could be justified. Whatever you decide, could you please send a PR? :)

MangelMaxime commented 5 years ago

The problem with starting to push breaking change for a prop is what if we want to strongly type another prop?

Do we release another major release? Unless we make sure to strongly type all the props that need to be strongly typed at once. But I am not sure, people will be happy with that major change.

alfonsogarciacaro commented 5 years ago

The problem with starting to push breaking change for a prop is what if we want to strongly type another prop?

Yeah, that's true. We would need someone to go through all the props and see which ones can be made strongly typed before pushing the stable release 😕

MangelMaxime commented 5 years ago

This is the reason why I mention the possibility to have another library for that.

My idea, is this is ok to replace obj with string or obj with int, etc. where we are sure that string and int, etc are the only correct types. So this doesn't break the code and make it a bit more secure.

And then have another library which strongly types everything, and is able to support units too etc.

alfonsogarciacaro commented 5 years ago

Fixed by #147, thanks @Zaid-Ajaj!