Improve the on* callback type signatures #14
Conversation
example/Example.res
Outdated
| @@ -57,7 +57,9 @@ module Form = { | |||
| render={({field: {name, onBlur, onChange, ref, value}}) => | |||
| <div> | |||
| <label> {name->React.string} </label> | |||
| <input name onBlur onChange ref value /> | |||
| <input | |||
| name onBlur={_event => onBlur()} onChange={Controller.handleEvent(onChange)} ref value | |||
There was a problem hiding this comment.
The Example is not relevant as in a "real world" project most of the native inputs would be nested in a few components. So the onBlur={_event => onBlur()} onChange={Controller.handleEvent(onChange)} boilerplate shouldn't be that annoying and is worth it considering the benefits.
| Js.Json.string( | ||
| value->Js.String2.split("")->Js.Array2.reverseInPlace->Js.Array2.joinWith(""), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Look ma, no setValue!
There was a problem hiding this comment.
it will reduce some hard time for ATS, of course I do know you dont really like this api from react-hook-form
There was a problem hiding this comment.
Not a big fan of how inconsistent the code is 😕
I would have loved to receive 2 different functions instead of a mysterious one XD
There was a problem hiding this comment.
agreed ! We hard to know the behavior of this one exactly if we dont check the implementation ( based on the doc, it just said event is any =)))
gaku-sei
force-pushed
the
improve-on_-callback-types
branch
from
e578944 to
8831786
Compare
src/Controller.res
Outdated
| /> | ||
| ``` | ||
| ") | ||
| let handleEvent = (onChange, event) => onChange(OnChangeArg.event(event)) |
There was a problem hiding this comment.
To me this one is really not necessary and kinda defy the original goal: to be runtime free.
Though, the runtime cost being very small I think we can keep it 😄
What do you guys think @vtrofin @infothien ?
There was a problem hiding this comment.
For me I prefer to just write
<Controller
render={({field: {onChange}}) =>
<input onChange={event => onChange(Controller.OnChangeArg.event(event))} />
}
/>
=)))
There was a problem hiding this comment.
Me too actually, i added this one as a compromise just in case XD
If @vtrofin is ok as well, let's drop this function 👍
There was a problem hiding this comment.
@infothien Actually I miss the good old days of onChange={onChange << Controller.OnChangeArg.event} that was much, much clearer 😞
There was a problem hiding this comment.
thank you @gaku-sei and @infothien for pointing this out. I'm onboard with the change as well
gaku-sei
force-pushed
the
improve-on_-callback-types
branch
from
8831786 to
c4688be
Compare
| Js.Json.string( | ||
| value->Js.String2.split("")->Js.Array2.reverseInPlace->Js.Array2.joinWith(""), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
it will reduce some hard time for ATS, of course I do know you dont really like this api from react-hook-form
|
Dropped |
First pr related to the big refactor: Improve the on callback type signatures*
The new types are closer to the actual code and allows for more flexibility. Not only is
onChangeeasier to call even without event, but it can now be used to update the value, no need forsetValue!