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

Fix improper type inference in hooks. #80

Closed
parkerziegler opened this issue Jun 25, 2019 · 4 comments
Closed

Fix improper type inference in hooks. #80

parkerziegler opened this issue Jun 25, 2019 · 4 comments

Comments

@parkerziegler
Copy link
Contributor

cc/ @gugahoa @Schmavery I'm not totally sure how to fix this one, so reaching out for help.

In examples/2-query in Monster.re, change L37 from this:

let (result, _) = Hooks.useQuery(~request, ());

to this:

useQuery(~request, ());

and add this at the top:

open Hooks;

When doing this, I get an interesting type error:

We've found a bug for you!
  /Users/parkerziegler/Documents/repos/OSS/reason-urql/examples/2-query/src/Monster.re 38:25-46
  
  36 ┆    value passed in from GetAll */
  37 ┆ let request = GetPokemon.make(~name=pokemon, ());
  38 ┆ let ({response}, _) = useQuery(~request, ());
  39 ┆ 
  40 ┆ switch (result.response) {
  
  This has type:
    ReasonUrql.Hooks.useQueryResponse({. "pokemon": option({. "classification": 
                                                             option(string),
                                                             "height": 
                                                             option({. "maximum": 
                                                                    option(
                                                                    string)}),
                                                             "image": 
                                                             option(string),
                                                             "name": 
                                                             option(string),
                                                             "weight": 
                                                             option({. "maximum": 
                                                                    option(
                                                                    string)})})})
      (defined as
      (ReasonUrql.Hooks.useQueryState({. "pokemon": option({. "classification": 
                                                             option(string),
                                                             "height": 
                                                             option({. "maximum": 
                                                                    option(
                                                                    string)}),
                                                             "image": 
                                                             option(string),
                                                             "name": 
                                                             option(string),
                                                             "weight": 
                                                             option({. "maximum": 
                                                                    option(
                                                                    string)})})}),
      ReasonUrql.Hooks.partialOperationContextFn))
  But somewhere wanted:
    (ReasonUrql.Hooks.useSubscriptionResponse('a), 'b)
  
  The incompatible parts:
    ReasonUrql.Hooks.useQueryState({. "pokemon": option({. "classification": 
                                                          option(string),
                                                          "height": option(
                                                                    {. "maximum": 
                                                                    option(
                                                                    string)}),
                                                          "image": option(
                                                                   string),
                                                          "name": option(
                                                                  string),
                                                          "weight": option(
                                                                    {. "maximum": 
                                                                    option(
                                                                    string)})})})
      (defined as
      UrqlUseQuery.useQueryState({. "pokemon": option({. "classification": 
                                                        option(string),
                                                        "height": option(
                                                                  {. "maximum": 
                                                                    option(
                                                                    string)}),
                                                        "image": option(
                                                                 string),
                                                        "name": option(
                                                                string),
                                                        "weight": option(
                                                                  {. "maximum": 
                                                                    option(
                                                                    string)})})}))
    vs
    ReasonUrql.Hooks.useSubscriptionResponse('a) (defined as
      UrqlUseSubscription.useSubscriptionResponse('a))

I'm not totally sure why this is happening. I'm wondering if Reason is searching for the record type associated with the destrucutred { response } and it's saying, "Hey, that record is type useSubscriptionResponse" That's my best guess. If so, I'm thinking we may need to do one-off module UseQuery = UrqlUseQuery; such that only the definitions from UrqlUseQuery are brought into scope and the proper record type useQueryResponse is inferred. Any other thoughts? I also tried adding a top-level type hookReponse('ret) since our hooks have consistent return structures, but I got an error w/ the useSubscription GADT b/c type ret there would escape its scope.

@Schmavery
Copy link
Collaborator

Yeah sounds like your assessment of the issue is right. I'm guessing it might work when you annotate the response like this:

let ({UrqlUseQuery.response}, _) = useQuery(~request, ());

One module per hook could help.. other ideas:

  1. Like you suggested, having a common hookResponse type (which I feel could be nicest except that it may require reverting the gadt trick?)
  2. Having different field names for the different hook responses? Like {queryResponse} and {subscriptionResponse}

@gugahoa
Copy link
Contributor

gugahoa commented Jun 25, 2019

I agree with the common hookResponse type, and tried to solve the issue here making that change. Going with the common type is still possible with useSubscription, maybe you got that error because type inference around GADT has to be more explicit, otherwise the inferred type won't be the desired one.

For the type inference to work with hookResponse (which I believe will be in UrqlTypes.re), you have to also add open Types below open ReasonUrql

@gugahoa
Copy link
Contributor

gugahoa commented Jun 25, 2019

I opened up a PR with the suggestion of a new type for hooks response: #81

@parkerziegler
Copy link
Contributor Author

:chefs-kiss: You two are literally the best OSS contributors I've ever worked with ❤️. I'm going to check internally about getting you write access to the repo so you can push branches directly, which I think will make it easier to test and verify one another's changes locally.

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

No branches or pull requests

3 participants