-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add uncurried hooks #551
Add uncurried hooks #551
Conversation
I think I was getting a little aggressive here. Since they are externals bs.uncurry should be enough. Please swap reducer back to [@bs.uncurry] and then rebuild to resolve conflicts and we can get this in! |
src/React.re
Outdated
|
||
[@bs.module "react"] | ||
external useReducer: | ||
((. 'state, 'action) => 'state, 'state) => ('state, (. 'action) => unit) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swap this input back.
src/React.re
Outdated
( | ||
[@bs.uncurry] (('state, 'action) => 'state), | ||
'initialState, | ||
'initialState => 'state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to your PR but I think this is unsafe. Should probably add bs.uncurry here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should go ahead and add [@bs.uncurry]
to that argument on the regular useReducerWithMapState
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. As a separate PR that would be great.
|
||
[@bs.module "react"] | ||
external useCallback: | ||
([@bs.uncurry] ('input => 'output)) => callback('input, 'output) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure you want to do all of the callback
type returns as well = https://reasonml.github.io/try?rrjsx=true&reason=C4TwDgpgBAxghgGwQIzjA1gCgOQEsA0U2A9gJRQC8RulAfEcQNwBQzA2gALIDOAdALbEAJgFcE0AEQAnCGmASAusyhQIAD2AQpAO0RQR3CAGFEKNOgBcylVEycevEdpgipUkAtt5tYEcDoMfr7ApOQU9PBIqBg4uD5+hCRBfmHWKhIGxqbR6BIszOL+AGbExJS2YfQA3gUQ-obARsRO-lSZJlHmmJiVUAAMpCwNTS09LAC+jEA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the Uncurried.useCallback*
functions should return type Uncurried.callback
, defined as type callback('input, 'output) = (. 'input) => 'output;
However I did just discover another bug. It doesn’t look like Reason is good at handling arity-zero uncurried functions. I’m not sure what the best way around that is, unless I’m missing something.
Seems to be related to reasonml/reason#2357
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I was oblivious. Good call for keeping continuity with the other set.
Yah that issue provides a workaround but it is kind of annoying that this error exists. I don't think it should block this PR though - if the feature is never used then a better experience won't be prioritized.
@johnridesabike if #574 gets merged, your diff will shrink to only two bindings per hook :-) |
@johnridesabike @rickyvetter all good here? Would love to get this in a state of mergeability 😄 |
@peterpme I don't have anything else I'm waiting for. Merging is fine with me as long as no one else has any issues. |
The original pull request for this got messed up, so this is re-opening it.
Original PR: #529
Fixes: #530