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

Expose all of urql context #64

Merged
merged 1 commit into from
Jun 7, 2019
Merged

Conversation

gugahoa
Copy link
Contributor

@gugahoa gugahoa commented Jun 6, 2019

Hello again! :)
I noticed only the Provider binding from urql was exposed here, whereas in the urql lib it exports Provider, Consumer and Context, so I created binding for those.

Is the intention to have 100% bindings from urql before 1.0?

@parkerziegler
Copy link
Contributor

Thanks @gugahoa! The intention is to get all of the hooks bound and be fully on the JSX 3 syntax. Once we land useSubscription (track it in #60) and Subscription has JSX 3 bindings, I'll cut a formal v1 bound to urql v1.1. From there I'd love help on docs (see response in #65) and tests! Tests are an especially interesting subject b/c these are really just bindings, so the fact that they compile is sort of a test in and of itself. I'd mostly love tests for our custom Reason helpers, and may consider some doing what bs-webapi-incubator does, where they just commit the compiled JS of some examples to source so the output can be visually verified.

@parkerziegler parkerziegler merged commit 2eab421 into teamwalnut:master Jun 7, 2019
@gugahoa gugahoa deleted the bind-context branch June 7, 2019 22:24
@gugahoa
Copy link
Contributor Author

gugahoa commented Jun 7, 2019

I have some thoughts about tests, but it pertains more about the type safety of the bindings. As they're right now, they're not type safe, so you can compile unsound code and it'll only blow up at runtime. (The hooks bindings, at least, the others I haven't taken a look yet)

Maybe it would be interesting to try and create tests for type safety?

@parkerziegler
Copy link
Contributor

🤔 Can you give an example of unsound code that would compile but blow up at runtime? I definitely like the idea of verifying type safety, but it'd be helpful to have an example of where things are going awry currently 😅

@gugahoa
Copy link
Contributor Author

gugahoa commented Jun 7, 2019

Sure, in 2-query replace Monster.re with this gist: https://gist.github.com/gugahoa/7a7afaea60ed9318f747f6a7d10d523b

It'll compile just fine, but if you try to render the component, it'll crash the React application.

@gugahoa
Copy link
Contributor Author

gugahoa commented Jun 7, 2019

That's because of the bindings to JS-land urql are using 'a in the return type. And not everything from JS can be safely cast to ReasonML/OCaml types.

I have been trying to think of ways to make it safer, there's three approaches I thought of:

  1. Document it better, so users at least know it can blow up
  2. Change the return form JS-land urql binding from generic 'a to a Js.t, and let users decode it into their own types (This made me go into a rabbit hole that made me write this blogpost: https://gustavoaguiar.dev/journey-trait-like-behavior-reasonml/)
  3. Make the return type from the binding equal to the already type-safe return type generated by graphql_ppx for our query

The approach 2 seems like it would make users add a lot of boilerplate if they want to have type-safe return values, the approach 3 seems like it would require some refactoring.

@parkerziegler
Copy link
Contributor

@gugahoa gotcha, thanks for the Gist and for being patient with the explanation. I like approach 3 best, and I think our components do support something like this, i.e. this seems to work:

module PatDog = [%graphql
  {|
    mutation patDog($key: ID!) {
      patDog(key: $key) {
        pats
      }
    }
  |}
];

<Mutation query=Mutations.patDog>
  ...{({executeMutation}: Mutation.mutationRenderProps(PatDog.t)) =>
    <EmojiButton
      emoji={j||j}
      count={string_of_int(pats)}
      hex="db4d3f"
      onClick={_ => executeMutation(Some(payload)) |> ignore}
    />
  }
</Mutation>

Is there any chance we can use this same approach for the hooks? For example, I believe this works already for useMutation:

module PatDog = [%graphql
  {|
    mutation patDog($key: ID!) {
      patDog(key: $key) {
        pats
      }
    }
  |}
];

let ({ response }: Hooks.useMutationResponse(PatDog.t), executeMutation) = Hooks.useMutation(~query=Mutations.treatDog);
  let pats = switch (response) {
    /* Attempting to access d##patDog##pat or d##patDog##treat will fail compilation. */
    | Data(d) => d##patDog##pats
    | _ => 0
  };

It does force users to pass in the type explicitly, but that's the point of type arguments 😅If we document this well, do you feel it would work as a nice API? I'm not totally sure outside of type arguments how we apply the graphql_ppx type w/out resorting to something like a functor.

@gugahoa
Copy link
Contributor Author

gugahoa commented Jun 10, 2019

@parkerziegler I have been looking for a way to do it in a type safe way, but I can't seem to find any documentation about a module signature that the output from graphql_ppx may follow, so it seems brittle to try and use module as arguments.
So I believe it would be best to have it documented so people know what they're doing at least.

@Schmavery Schmavery mentioned this pull request Jun 13, 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.

2 participants