Skip to content
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 more useState flavours #426

Closed
wants to merge 1 commit into from

Conversation

jchavarri
Copy link
Collaborator

This is mostly a discussion / question PR, related to #402.

Something like useStateValue below is really useful when one starts prototyping a component, and it feels like ReasonReact should provide an out-of-the-box solution for it.

Maybe there is some typing edge case that I'm missing? I read this comment but I can't figure out in which cases this would not be safe.

@rickyvetter
Copy link
Contributor

This isn't typesafe if 'state is a function. I don't think we want to support this API (nor do I think it's the right api in JS).

@jchavarri
Copy link
Collaborator Author

@rickyvetter Could you help me understand how safety is broken, please? I'm having a hard time finding an example of how that'd look like, I thought that separating the different cases with one external for each would solve any typing issues. thanks!

@jchavarri
Copy link
Collaborator Author

n/m, I discussed with @cristianoc and understood the issue is that if state is a function of type unit => 'a, then useState will "interpret" that is the "lazy" version of useState and it'll behave differently.

Alternatives:

  1. Define useStateValue and useStatePrevious on top of useState, so the lazy version is always used, and the behavior on JS side remains always the same. I guess this is a no-go as there's no way (that I know of) to do this without giving up on the "zero-cost" bindings policy.
  2. Keep the existing API.
  3. Polish this PR and adopt something like this.

I (unsurprisingly 😄 ) think we should go with 3. This is an issue in ReactJS API design, and I don't think it's pragmatic that ReasonReact users pay the cost to avoid the confusion for such a niche case by being offered a subset of the original API, which also happens to be the one that is least commonly needed.

If anyone really needs to store a function unit=>'a as state, they can use the already existing useState, but for all other cases, which will be the large majority, useStateValue and useStatePrevious are available (we can change/ improve the naming if needs be).

@rickyvetter
Copy link
Contributor

I can't justify shipping code that's unsafe in this way for people even in edge cases, so 3 really isn't an option for me. 1 is something that I am open to. A couple options would be to have another module like React.Utils or ReactUtils which builds off of the no runtime core implementation. I am uneasy about shipping with reason-react - would be interested in thoughts here.

Sorry for going silent on this and many other issues. Getting more involved here again.

@jchavarri jchavarri deleted the usestate-additions branch October 13, 2020 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants