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 common hook type (Fix #80) #81

Merged
merged 6 commits into from
Jun 28, 2019

Conversation

gugahoa
Copy link
Contributor

@gugahoa gugahoa commented Jun 25, 2019

No description provided.

@Schmavery
Copy link
Collaborator

This is a super slick solution afaict, nice. Any thoughts about reexposing the result type (and any other relevant types) from the ReasonUrql.Hooks module so that users don't have to open another module?

src/UrqlTypes.re Outdated Show resolved Hide resolved
Copy link
Contributor

@parkerziegler parkerziegler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @gugahoa! I agree with @Schmavery, is there a way for us to essentially re-export Types in the Hooks module so that it doesn't have to be opened by the user? Would it be enough to just include UrqlTypes in the definition of module Hooks? Just ping here once you've made the few requested changes 🤗 ❤️

@gugahoa
Copy link
Contributor Author

gugahoa commented Jun 27, 2019

I tried to make it not necessary to open Types when opening Hooks, but couldn't accomplish it, any ideas on how to do it?

I tried making a type hookResponse('ret) = UrqlTypes.hookResponse('ret) in each file, or putting it inside module Hooks

Pinging @parkerziegler for re-review

@Schmavery
Copy link
Collaborator

Schmavery commented Jun 27, 2019

@gugahoa I'm surprised that adding type hookResponse('ret) = UrqlTypes.hookResponse('ret) to the Hooks module didn't work. I was playing with it a little earlier, does this help?
https://reasonml.github.io/try?rrjsx=true&reason=LYewJgrgNgpgBAFQJ4AcYGc4F44G8BQccALqvAIbZ4CWAXHNQHbEC+A3ISWXAEbadEAPnABCIAB4AKJsQCUHdvnyhIsOAAkQIANaYcBIqTRxKOZGnQA6UzXozFh7nzNkrzuMLFSZ8pURXQ8ACCVAZEcLDEcABmVJK45hiW1Cyy2AB8DBxEipwBaiKhAhEwUbE48YlWKWlYmdTZcIq5IGiMGlq6HJEkoXRwAIyKPcQATFRekgO+QZbRknIcQA

Basically I reexposed the type using something like this in the Hooks module:

type a = Types.a = {i: int};

@parkerziegler
Copy link
Contributor

@gugahoa can you try the above suggested by @Schmavery? If we get that to work then I think this is set to 🚢

@parkerziegler
Copy link
Contributor

parkerziegler commented Jun 27, 2019

Another thing to bring up while I'm here – do you happen to know how to handle GADTs in the component case? I got it working-ish w/ Subscription, but this syntax will fail the build:

[@react.component]
let make =
    (
      type acc,
      type response,
      type ret,
      ~request: request(ret),
      ~handler: handler(acc, response, ret),
      ~children: subscriptionRenderProps(ret) => React.element,
    ) => {

with the following error:

[1/2] Building src/components/UrqlSubscription.mlast
Fatal error: exception Invalid_argument("react.component calls can only be on function definitions or component wrappers (forwardRef, memo).")
ninja: error: rebuilding 'build.ninja': subcommand failed
error Command failed with exit code 2.

I wonder if there's any easy way around this 🤔

@gugahoa
Copy link
Contributor Author

gugahoa commented Jun 28, 2019

@Schmavery thanks for the suggestion, it worked perfectly!
@parkerziegler I remember getting stuck on that error previously, but I don't remember what caused it. Is your work in any branch where we can take a look?

@parkerziegler
Copy link
Contributor

parkerziegler commented Jun 28, 2019

Yep, just pushed it up: https://github.com/FormidableLabs/reason-urql/tree/task/type-inference-gadt-for-subscription Here are the relevant lines: https://github.com/FormidableLabs/reason-urql/blob/task/type-inference-gadt-for-subscription/src/components/UrqlSubscription.re#L53-L80

Basically, I think the [@react.component] flag enforces nothing other than named args in the function signature. It's not the end of the world if we ship one hook and 2 components as well – sometimes that's just the breaks.

@gugahoa
Copy link
Contributor Author

gugahoa commented Jun 28, 2019

@parkerziegler seems like you found a bug, I tried to look for any issues that mentioned that, but couldn't find, so I created a new one on the reason-react repo: reasonml/reason-react#452

@parkerziegler
Copy link
Contributor

Thanks @gugahoa. I'm skeptical that we'll get support for it in the near future, so I'll push up a possible solution for us so that we can feature cap v1.

@parkerziegler parkerziegler merged commit d434fbd into teamwalnut:master Jun 28, 2019
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.

3 participants