Proposed API change

[jfrolich]

This is a WIP, but I found that the functor API created quite a bit of boilerplate code. When I looked at the source of reason-urql, it found a way to implement a more concise API, so I messed around with the source of reason-react-hooks if we can have something like that that with apollo and this is the result. I would like to get your opinion and possible improvements.

In short we can go from this:

module ActivityPhotosQueryConfig = [%graphql
  {|
  query ActivityPhotos($feedId: ID, $scaleFactor: Float!, $height: Float!) {
    feedImageList(feedId: $feedId) {
      nodes {
        id
        width
        height
        file(scaleFactor: $scaleFactor, height: $height) {
          url
        }
      }
    }
  }
|}
];
module ActivityPhotosQuery =
  ReasonApolloHooks.Query.Make(ActivityPhotosQueryConfig);

[@react.component]
let make = (~feedId=?, ~small=false, ~edit=false) => {
  let height = small ? 100. : 200.;
  let (query, _full) =
    ActivityPhotosQuery.use(
      ~variables=
        ActivityPhotosQueryConfig.make(
          ~feedId?,
          ~scaleFactor=PixelRatio.get(),
          ~height,
          (),
        )##variables,
      (),
    );
}

to this:

module ActivityPhotosQuery = [%graphql
  {|
  query ActivityPhotos($feedId: ID, $scaleFactor: Float!, $height: Float!) {
    feedImageList(feedId: $feedId) {
      nodes {
        id
        width
        height
        file(scaleFactor: $scaleFactor, height: $height) {
          url
        }
      }
    }
  }
|}
];

[@react.component]
let make = (~feedId=?, ~small=false, ~edit=false) => {
  let height = small ? 100. : 200.;
  let (query, _full) =
    ReasonApolloHooks.useQuery(
      ~query=
        ActivityPhotosQuery.make(
          ~feedId?,
          ~scaleFactor=PixelRatio.get(),
          ~height,
          (),
        ),
      (),
    );
}

Similar for mutations, however we need to pass in an initial mutation if we want to provide variables later:

let (create, _simple, _full) =
  ReasonApolloHooks.useMutation(
    ~incompleteMutation=CreateFeedImageMutation.query,
  (),
);

We can run the create function like this:

create(
  ~mutation=CreateFeedImageMutation.make(~feedImage, ()),
  ~refetchQueries=_ => [|refetchQuery(~feedId?, ())|],
  ~awaitRefetchQueries=true,
  (),
)

or we can add the mutation from the get go (and then we don't have to supply it later):

let (create, _simple, _full) =
  ReasonApolloHooks.useMutation(
    ~mutation=CreateFeedImageMutation.make(~feedImage, ()),
  (),
);

To be honest the mutation API is suboptimal. I think there is a way that we do not need to provide the incompleteMutation but this involves changing the javascript useMutation hook. OR we need to modify graphql_ppx, to generate a different module.

[fakenickels]

Thanks for your proposal! I think we can leave this opened for a while to see what others will say about it

[jfrolich]

Sounds great. I am pretty new to Reason/OCaml, so there are probably many things that can be better.

[fakenickels]

No opinions of otherss so far but in overall I liked the simplification. Can you update the examples? After that I think we are good to go

[jfrolich]

I updated the PR, a few changes:

• Changed the main module to ApolloHooks (debatable, but more concise it's possible now that the package is not namespaced anymore)
• Updated graphql_ppx in the example to graphql_ppx_re (which is a maintained version of graphql_ppx in reason -- same API).
• Removed the namespacing so we can do ApolloHooks.useQuery directly instead of ApolloHooks.Query.useQuery or something. Also have all the hooks available when opening ApolloHooks.

[fakenickels]

Thanks a lot @jfrolich ! These next upcoming weeks we'll be really busy for me but I'll try to review and merge ASAP

[jfrolich]

Thanks a lot @jfrolich ! These next upcoming weeks we'll be really busy for me but I'll try to review and merge ASAP

Cool. No worries. When this is merged I also have a pull-request that allows to do fetchMore in pure reason, instead of the raw bucklescript escape hatch!

[sgrove]

This looks fantastic!

We've added code-gen support for the current API, you can see the current usage for both queries and mutations here: https://serve.onegraph.com/short/B8DR68 (open code exporter and choose Reason as the language)

I'd love to get this merged in and make the code that much more approachable for workshops (and for me too of course!).

[fakenickels]

Amazing!! Will get this merged before weekend 🙏

[jfrolich]

This looks fantastic!

We've added code-gen support for the current API, you can see the current usage for both queries and mutations here: https://serve.onegraph.com/short/B8DR68 (open code exporter and choose Reason as the language)

I'd love to get this merged in and make the code that much more approachable for workshops (and for me too of course!).

Wow that codegen is awesome!

[jfrolich]

Amazing!! Will get this merged before weekend 🙏

I merged in the upstream changes and upgraded graphql_ppx_re in the example.

[fakenickels]

Merged will release soon!

[fakenickels]

Thanks lot @jfrolich for this!! Awesome work. And thanks everyone for giving feedback

[MargaretKrutikova]

I am a bit late to the party, but what happened to errorPolicy in useQuery? I think it disappeared.

[fakenickels]

Looks like we have some regressions here indeed, along with #57

[fakenickels]

We haven't released this just yet so there is still time to fix

[MargaretKrutikova]

I will try to run the example project locally, and see what I can fix 😄

[fakenickels]

I think will be better to revert this and open the PR again so we can make we have less breaking changes

[fakenickels]

@jfrolich would you mind reopening this PR?

[jfrolich]

Sorry about this. I could directly merge master because due to filename changes it wouldn't diff the fires. So I looked up the individual commits and applied them manually, but I messed up and missed two additions!

I will re-open this PR, and fix the issues as reported.

Some comments of @MargaretKrutikova are about existing code not related to this PR. This can be tackled in a separate PR. (Except the naming of incompleteMutation, happy to change this name if anyone has a name that is better suited or a way so we don't need to pass in the incomplete mutation.

[fakenickels]

@all-contributors add @jfrolich code, review and ideas

[allcontributors[bot]]

@fakenickels

I've put up a pull request to add @jfrolich! :tada: