Change type of Value prop

fable-compiler / fable-react

Fable bindings and helpers for React and React Native

MIT License

275 stars 67 forks source link

Change type of Value prop #123

Closed alfonsogarciacaro closed 5 years ago

alfonsogarciacaro commented 5 years ago

When using a select element that has Multiple true property, React forces us to pass a string array as Value, however this prop is currently typed as Value of string, which makes unboxing a needed (after the dev discover the error at runtime).

We have two solutions to fix this:

Make it obj as we have done for other items, I assume the runtime will automatically transform things as numbers to string, but it may not be the format they expect.
Make it U2<string, string[]>, which will break a lot of code forcing devs to add !^ everywhere.

What do you think? @MangelMaxime @nojaf

nojaf commented 5 years ago

Hmm, I guess in both cases there is a possibility that the user can make mistakes. With Multiple false you still allow the use of an array.

Perhaps you need a select_multiple function and remove the Multiple property all together. Would not break the select function that much but introduce some duplication.

MangelMaxime commented 5 years ago

@alfonsogarciacaro Hum...

I do like the fact that we are force to use string for Value. For example, if you works with number and make Value of obj then users would be tempted to pass an int or float value directly and I am not sure it will have the correct behavior.

Perhaps, we can provide something like [<CompiledName("value")>] MultipleValue of string [] ?

MangelMaxime commented 5 years ago

Also using U2<string, string[]> will make the code more cryptic and harder to write.

input [ Value !^"my-value"]

IMHO we should avoid this at all cost :)

alfonsogarciacaro commented 5 years ago

Hmm, select_multiple could work but although we're adding helpers for other things like React.memo I'd prefer to keep the HTML native elements untouched so things like html-to-elmish work. I like the solution of adding a different prop, maybe ValueMultiple so it works better with autocompletion, let's do that!

alfonsogarciacaro commented 5 years ago

Actually, I just noticed I had turned Value of string to Value of obj and month ago and I had forgotten 😅 Anyways, I've changed it back to string and added ValueMultiple of string[]. Not sure how it can work with SSR exactly so for now I just disabled it.