How to make updateQuery type safe?

[fakenickels]

Currently there is no way to not to avoid to use a escape hatch bs.raw to do the logic.

Probably we could recommend the usage of https://github.com/jaredly/js_deep_ppx?
And of course change the types of updateQuery to use (prev: Query.t, {. "fetchMoreResult": Query.t}) => Query.t

[fakenickels]

I could successfully use js_deep_ppx in this project I'm working on

let concatResults: (TransactionsQuery.t, { . "fetchMoreResult": TransactionsQuery.t }) => TransactionsQuery.t = (prev, next) => {
  let getEdges = obj => obj##currentUser->Belt.Option.flatMap(user => user##transactions##edges)->Belt.Option.getWithDefault([||]);
  let fetchMoreResultEdges = getEdges(next##fetchMoreResult);
  let previousEdges = getEdges(prev);
  let newEdges = Belt.Array.concat(previousEdges, fetchMoreResultEdges);

  [%js.deep prev["currentUser"].map(
    user => user->Belt.Option.map(user => user["transactions"]["edges"].replace(Some(newEdges)))
  )]
};

[MargaretKrutikova]

I am not sure updateQuery will give us prevResult of type Query.t, since it is read directly from the cache, it might not be the same as we specify in the graphql_ppx with bsRecord or bsDecode. I think the same is true for fetchMoreResult.

If we want them to be of type Query.t we will need to parse them at some intermediate point, but then we can't write them back into cache without serializing to the original JS objects, which we can't really do 😕

The problem with different types in the apollo cache and what we get after parsing the response via graphql_ppx seems really tough, and also makes it so frustrating to use cache, would be really nice to have normal reason types there 😄

[fakenickels]

Fair point, wasn't recalling that the cache data can be slightly different from the result we consume – even more if our users are making use of bsRecord and bsDecode as you said.

Probably we would need a way to use graphql_ppx_re's parse function and then come up with a unparse to get a raw version of the data

[fakenickels]

would be really nice to have normal reason types there

I would love to have that 😓 , it's just way too error-prone with raw JS

[MargaretKrutikova]

There is this PR in graphql_ppx_re, that implements the serialize function. There are however some tricky things with bsDecoder, since it would require the user to provider some kind of encoder too, @baransu described it in the PR description.

I was also thinking about the overhead of serializing/parsing data to and fro. Like in case of fetchMore it would need to parse and then serialize the whole list items that are already in cache, wouldn't it introduce some performance overhead?

On the other hand, if you only use bsRecord, you won't really need to convert anything, since it will be the same JS objects.

[fakenickels]

I was also thinking about the overhead of serializing/parsing data to and fro. Like in case of fetchMore it would need to parse and then serialize the whole list items that are already in cache, wouldn't it introduce some performance overhead?

Probably it would, just not sure if would be too much of it. We can totally benchmark and run some tests when that PR lands in graphql_ppx_re, will solve all of our problems 🙏 .

Another way could be to have a "pure JS" Query.t from graphql_ppx_re without the parse modifications to be easier to reconcile with the serialized version

[mbirkegaard]

We use both reason-apollo-hooks and graphql_ppx_re in production and need both updateQuery and optimisticResponse (The latter is in our fork. Will create a PR after getting our changes updated for the new PR). I've was about to open an issue about this exact question.

Another way could be to have a "pure JS" Query.t from graphql_ppx_re without the parse modifications to be easier to reconcile with the serialized version

I think this is the easiest way to get there. It's at least way more more ergonomic and less error-prone than manually translating to a js object. I.e. we can get graphql_ppx_re to expose types that precisely describe the shape of the object expected by apollo.

We're still experimenting with patterns to find good ones, but I find that what I end up doing when working directly with the apollo cache is to write out the types anyway, like so:

  module FetchMore = {
    type resultJson('conversation) = {
      .
      "conversationsV2": {
        .
        "results": array({.. "id": string} as 'conversation), // Has more attributes, but only need id
        "pageInfo": {
          .
          "lastCursor": string,
          "hasNext": bool,
          "__typename": string,
        },
        "__typename": string,
      },
    };

    external decode: Js.Json.t => resultJson(_) = "%identity";
    external encode: resultJson(_) => Js.Json.t = "%identity";

    let updateQuery: ReasonApolloHooks.Query.updateQueryT =
      (prevResult, updateQueryOptions) => {
        updateQueryOptions
        ->ReasonApolloHooks.Query.fetchMoreResultGet
        ->Belt.Option.map(fetchMoreResult => {
            let prevResult = prevResult->decode;
            let fetchMoreResult = fetchMoreResult->decode;
            {
              "conversationsV2": {
                "results":
                  Belt.Array.concat(
                    prevResult##conversationsV2##results,
                    fetchMoreResult##conversationsV2##results
                    ->Belt.Array.keep(newResult =>
                        !
                          Belt.Array.some(
                            prevResult##conversationsV2##results, prevResult =>
                            prevResult##id == newResult##id
                          )
                      ),
                  ),
                "pageInfo": fetchMoreResult##conversationsV2##pageInfo,
                "__typename": prevResult##conversationsV2##__typename,
              },
            }
            ->encode;
          })
        ->Belt.Option.getWithDefault(prevResult);
      };
  };

updateQuery, readQuery, writeQuery and optimisticResponse would all make use of this pure JS type, which could then be passed to parse if needed. Having __typename handled automatically would be a nice boon, but even without it this would be a tremendous help.