Skip to content
This repository has been archived by the owner on Jul 6, 2020. It is now read-only.

Support simplePagination cases as automatic resolver with cache misses supported #110

Closed
strblr opened this issue Oct 30, 2019 · 20 comments · Fixed by #115 or #143
Closed

Support simplePagination cases as automatic resolver with cache misses supported #110

strblr opened this issue Oct 30, 2019 · 20 comments · Fixed by #115 or #143
Labels
question 🙋 A question or usage-issue that won’t need changes

Comments

@strblr
Copy link

strblr commented Oct 30, 2019

I just discovered urql and it's pure gold. This library was really needed. Apollo is great but monopolies always have their downsides.

I can't wait to switch to urql in my main big professional project. I have carefully read the doc to see how I would convert the Apollo features I use to urql. Everything is pretty much there (smart graph cache, updating cache on mutation, custom cache keys, etc) except maybe two things :

  • Something that would do the same job as fetchMore,
  • Support for the @connection directive.

For fetchMore, I can't handle my own custom state as I need the cache as my single source of truth, and I can't use the graphcache because from what I read there's currently no way to customize the merging of query results.

In my project, I have around 30 list queries with @connection directives. They don't follow the relay spec and look more like this :

query Comments($projectId: ID!, $parent: ID, $search: String, $sort: String, $sortOrder: Int, $limit: Int!, $page: Int!) {
  comments(projectId: $projectId, parent: $parent, search: $search, sort: $sort, sortOrder: $sortOrder, limit: $limit, page: $page)
    @connection(key: "comments", filter: ["projectId", "parent"]) {
    total
    items {
      id
      ...
    }
  }
}

To give you an idea, here is what my current fetchMore does :

fetchMoreImpl({
  variables: {
    ...variables,
    page: Math.ceil(comments.items.length / variables.limit) + 1
  },
  updateQuery(prev, { fetchMoreResult }) {
    if(!fetchMoreResult) return prev
    return {
      comments: {
        ...prev.comments,
        items: [
          ...prev.comments.items,
          ...fetchMoreResult.comments.items
        ]
      }
    }
  }
})

It would be really nice to have something for this ! I would switch to urql in the blink of an eye.
Thank you for your amazing work.

@JoviDeCroock
Copy link

JoviDeCroock commented Oct 30, 2019

Hey, thanks for the kind words

I think linear to the relayPagination a resolver can be implemented to handle this concatenation for you, That should fix the issue with needing fetchMore. This custom resolver in turn removes the need for @connection.

@kitten
Copy link
Member

kitten commented Oct 30, 2019

Hiya 👋

We haven't added support for @connection and fetchMore because our approach to both of these things are very different.

Instead of adding @connection we actually more powerful "cache resolvers" than Apollo's InMemoryCache. Instead of just supporting cacheRedirects our resolvers can be used to resolve any kind of data in a full server-like resolver: https://github.com/FormidableLabs/urql-exchange-graphcache/blob/master/docs/resolvers.md

Now, the question is obviously, how does that come together to a full pagination implementation? We've actually already implemented a helper for automatic Relay pagination: https://github.com/FormidableLabs/urql-exchange-graphcache/blob/master/src/extras/relayPagination.ts (https://github.com/FormidableLabs/urql-exchange-graphcache/blob/master/docs/resolvers.md#pagination)

This can actually be replicated for any kind of pagination, although we haven't written a doc on how to do this yet

Edit: Also to clarify, this approach has a significant advantage. We're never manipulating the cache data to be different from the API's data. Instead we're basically manipulating only what Graphcache resolves and stitch pages together.

@kitten kitten added the question 🙋 A question or usage-issue that won’t need changes label Oct 30, 2019
@strblr
Copy link
Author

strblr commented Oct 30, 2019

Alright, I'm going to give it a try on a smaller test project and quickly come back to you. Thank you for your guidance.

@kitten
Copy link
Member

kitten commented Oct 30, 2019

I’m also thinking that we may want to provide a simplePagination helper somehow, to make non-Relay pagination easier, but it may be hard to generalise those.

But maybe we can offload some of the complexity around generalising this by making it accept a function in its config that is user-defined and merges two pages 🤔

@strblr
Copy link
Author

strblr commented Oct 30, 2019

Actually, if I'm able to come up with something nice on my side, I can try and propose you a generic pagination utility. I really like this project so far and would be pleased to contribute.

@strblr
Copy link
Author

strblr commented Oct 31, 2019

I think something that would really help is a method on the cache to rebuild a complete response from a cache key (in this case a page). I see that it can be done by recursively traversing cache.links and cache.records. Without it, I think it will be pretty complicated to go generic without forcing users to go really low-level for each type of pagination they would like to use (like in relayPagination), in which case they pretty much don't need an external utility function.

I don't know how Apollo's normalized cache manages to restitue full json pages in fetchMore or readQuery for example.

@strblr
Copy link
Author

strblr commented Oct 31, 2019

@JoviDeCroock @kitten

If I understand it right, relayPagination does a controlled aggregation of all the previous queries having the same signature minus the keys used for pagination. I think that raises one issue, more or less linked to my previous comment (I'll come back to that) : not all variable changes should end up creating a new entry in the cache. Sorry if I always come back to Apollo, but it's my only reference point when it comes to the subject : in Apollo @connection actually controls the cache entry being created. If another query result flies in with the same filters variables, the previous cache entry is replaced. There is no such thing in exchange-graphcache, which can lead to a very uselessly high amount of data being stored.

For example, if I have the following query where search is a weak variable (ie ignored by Apollo's cache) :

query Users($domain: ID, $search: String, $limit: Int!, $page: Int!) {
  users(domain: $domain, search: $search, limit: $limit, page: $page) {
    total
    items {
      id
      name
      email
      website
      domain
    }
  }
}

Changes in search and limit should not result in a new cache entry. Neither for page, even if that would make a little more sense. In my app I have a lot of these queries, and each one is controlled by an input for the searching, while typing. With @connection, only one cache entry is used. In exchange-graphcache, a new entry is created on each new character being typed in.

Semantically, this doesn't make sense. By typing "chocolate" in the search bar, the user doesn't need to keep cache results for "cho", "choc", "chocol", etc. In my opinion, the core underlying problem is that graphcache in fact doesn't distinguish weak and strong variables. That's the reason why everything else done afterwards in custom resolvers will feel hacky (please see previous comment), besides the storage problem mentioned earlier. That's maybe why, fundamentally, custom resolvers may not be a complete replacement for @connection. They're great but solve different problems I guess, and having custom resolvers may not undercut the need for a @connection-like feature.

It would be really nice if there was some way to intervene upstream, like a way to control the actual cache entries being created. Actual support for @connection or something like :

cacheExchange({
  connections: {
    Query: {
      users: ["domain"],
      comments: ["chatId", "parent"]
    }
  }
})

Resulting in other variables being ignored when generating the cache keys.

What do you guys think ?

@kitten
Copy link
Member

kitten commented Oct 31, 2019

First and foremost, sorry, but I don’t think we will go with connections 😅 Personally, I think it’s not a good API and having the cache store “fantasy” data that is stitched together is not that great, as we’re moving farther from creating a source of truth, to creating a store without actually deriving data.

Luckily, it’s still possible to do this with our current approach. It’s just somewhat hidden in the relayPagination source.

The “secret” is that similar constraints apply to Relay-style pagination. All arguments that aren’t related to pagination are assumed to create different pages.

When we’re merging all the pages we know about we’re actually only looking at the ones that have matching variables that we care about, and this is a little hidden in the source unfortunately.

One part of this is this line: https://github.com/FormidableLabs/urql-exchange-graphcache/blob/b1e86c517abea9c09f9bc96b176aef713596614f/src/extras/relayPagination.ts#L149
With resolveConnections we actually retrieve a list of all the possible variables and links that this specific field knows about. So we can basically ask: “What arguments have you cached this field with before?”

The other important part is then this line: https://github.com/FormidableLabs/urql-exchange-graphcache/blob/b1e86c517abea9c09f9bc96b176aef713596614f/src/extras/relayPagination.ts#L162
On this line we’re comparing the cached arguments on the field (so for a single page) with the input arguments for the current query.
And only when the arguments that aren’t related to pagination all match, we merge the results into our merged page result.

Hope this makes sense so far 👍 It is possible, and if all else fails, I’m happy to take a shot at making a nice simplePagination helper once I find the time 🙌

@strblr
Copy link
Author

strblr commented Oct 31, 2019

Yes I understand your strategy, it's pretty clear from the code which is well written by the way, but I don't think this is any cleaner than @connection. You just defer the pagination aggregation, but I don't see what you really gain. Useless and outdated cache keys are kept in the store and the aggregation complexity is left in the hands of the developer.

@kitten
Copy link
Member

kitten commented Oct 31, 2019

@ostrebler Right now it’s not actually a matter of gain vs drawback, it’s more of an approach and philosophical choice of how we’ve structured and built the cache.

The one advantage however would be that you can still retrieve individual pages, since the cache isn’t being changed to write completely different data.

This also means that offline support is improved drastically (a future ambition of ours). If we wrote arbitrary data to the cache that isn’t server data, how would you deploy a different version of the app that writes different data or attempts to remove the connection? In such a case all the old data suddenly becomes invalid.

We just don’t believe that it should be possible to always write data to the cache that is just what the UI happens to need. We’d like to keep that data as normalised as possible and let resolvers do some of the heavy lifting of transforming that data.

Useless and outdated cache keys are kept in the store

As long as we don’t have invalidation and GC figured out, both approaches leave outdated data in the cache. We do have some invalidation that can be triggered from mutations, which clean up a lot of invalidated data, but this is a separate problem

aggregation complexity is left in the hands of the developer

Well, this is what we’re trying to solve, isn’t it? With relayPagination everything’s already 100% automatic. As a user you just add a single boilerplate line to the config.

I would really like the same for non-Relay pagination, which is why I’m talking about a simplePagination helper

@sporieg
Copy link

sporieg commented Nov 13, 2019

Is there progress on the simplePagination helper?

I have a similar issue, where I need to page by limit/offset, but I am not having much luck figuring out how to implement the resolver.

@JoviDeCroock
Copy link

@sporieg work is happening on the /simplePagination branch

@JoviDeCroock
Copy link

Hey, sorry for the late update.

Have you tried using simplePagination? It's available in the 1.2.x releases

@sporieg
Copy link

sporieg commented Nov 25, 2019

Late reply, but I am having issues with the implementation. It behaves different for the same integration test depending on if that test is in the @urql/exchange-graphcache codebase or my own.

The test scaffold looks like:

import {Client, createClient, dedupExchange, fetchExchange, OperationContext, OperationResult} from "urql";
import {pipe, Source, take, toPromise} from "wonka";
import gql from 'graphql-tag';
import {AxiosInstance,  AxiosRequestConfig, AxiosResponse, Method} from "axios";
import {simplePagination} from "@urql/exchange-graphcache/extras";
import {cacheExchange} from "@urql/exchange-graphcache";
const axios: AxiosInstance = require("axios").default;

const testContext = {
  ["fetch"]: (url: string, r: RequestInit): Promise<any> => {
    const cfg: AxiosRequestConfig = {
      data: r.body,
      headers: r.headers,
      method: r.method as Method
    };
    return axios(url, cfg)
      .then(({status, statusText, data}: AxiosResponse) => {
        return {
          status,
          statusText,
          json: () => {
            return new Promise(res => {
              res(data)
            })
          }
        };
      })
  }
};

const query = gql`
    query List($from: Int, $limit: Int) {
        devices(from: $from, limit: $limit) {
            field
        }
    }
`;
function asPromise(
  result: Source<OperationResult<any>>
): Promise<any> {
  return pipe(
    result,
    take(1),
    toPromise
  );
}

test("northwind", async () => {
  const gql: Client =   createClient({
    url: "myurl",
    exchanges: [
      dedupExchange,
      cacheExchange({
        resolvers: {
          Query: {
            devices: simplePagination({
              limitArgument: 'limit',
              offsetArgument: 'from'
            })
          }
        }
      }),
      fetchExchange
    ]
  });
  const result: Source<OperationResult<any>> = gql.executeQuery({
    key: 0,
    query: query,
    variables: {
      limit: 10,
      from: 0
    }
  }, testContext as Partial<OperationContext>);
  const data: OperationResult<any> = await asPromise(result);
  if(data.data) {
    console.log(JSON.stringify(data, null, 2));
    expect(data.data.devices).toHaveLength(10);
  } else {
    fail(data.error)
  }
});

When it fails, I get the following values for Source<OperationResult<any>>

console.log test/blah.test.ts:92
    {
      "operation": {
        "key": 0,
        "query": {
          "kind": "Document",
          "definitions": [
            {
              "kind": "OperationDefinition",
              "operation": "query",
              "name": {
                "kind": "Name",
                "value": "List"
              },
              "variableDefinitions": [
                {
                  "kind": "VariableDefinition",
                  "variable": {
                    "kind": "Variable",
                    "name": {
                      "kind": "Name",
                      "value": "from"
                    }
                  },
                  "type": {
                    "kind": "NamedType",
                    "name": {
                      "kind": "Name",
                      "value": "Int"
                    }
                  },
                  "directives": []
                },
                {
                  "kind": "VariableDefinition",
                  "variable": {
                    "kind": "Variable",
                    "name": {
                      "kind": "Name",
                      "value": "limit"
                    }
                  },
                  "type": {
                    "kind": "NamedType",
                    "name": {
                      "kind": "Name",
                      "value": "Int"
                    }
                  },
                  "directives": []
                }
              ],
              "directives": [],
              "selectionSet": {
                "kind": "SelectionSet",
                "selections": [
                  {
                    "kind": "Field",
                    "name": {
                      "kind": "Name",
                      "value": "devices"
                    },
                    "arguments": [
                      {
                        "kind": "Argument",
                        "name": {
                          "kind": "Name",
                          "value": "from"
                        },
                        "value": {
                          "kind": "Variable",
                          "name": {
                            "kind": "Name",
                            "value": "from"
                          }
                        }
                      },
                      {
                        "kind": "Argument",
                        "name": {
                          "kind": "Name",
                          "value": "limit"
                        },
                        "value": {
                          "kind": "Variable",
                          "name": {
                            "kind": "Name",
                            "value": "limit"
                          }
                        }
                      }
                    ],
                    "directives": [],
                    "selectionSet": {
                      "kind": "SelectionSet",
                      "selections": [
                        {
                          "kind": "Field",
                          "name": {
                            "kind": "Name",
                            "value": "field"
                          },
                          "arguments": [],
                          "directives": []
                        },
                        {
                          "kind": "Field",
                          "name": {
                            "kind": "Name",
                            "value": "__typename"
                          }
                        }
                      ]
                    }
                  }
                ]
              }
            }
          ],
          "loc": {
            "start": 0,
            "end": 123
          }
        },
        "variables": {
          "limit": 10,
          "from": 0
        },
        "operationName": "query",
        "context": {
          "url": "...",
          "requestPolicy": "cache-first",
          "meta": {
            "cacheOutcome": "hit"
          }
        }
      },
      "data": {
        "__typename": "Query",
        "devices": null
      }
    }

I have run this test in:

  • @urql/exchange-graphcache - passes
  • My companies react app - fails
  • A new bare bones typescript repo created from tsdx - fails

I am going to keep testing around.

@JoviDeCroock
Copy link

That sounds like a setup problem to me, it’s saying cache hit without data in cache. that could suggest that we are having a false positive there.

@JoviDeCroock JoviDeCroock reopened this Nov 26, 2019
@kitten
Copy link
Member

kitten commented Dec 8, 2019

Sorry for the delay here 😓

That sounds like a setup problem to me, it’s saying cache hit without data in cache. that could suggest that we are having a false positive there.

I guess this is "expected behaviour". When no schema is passed there can never be a cache miss from resolvers. We may want to get rid of that:
https://github.com/FormidableLabs/urql-exchange-graphcache/blob/d668ae80dac2a14dc601ae44679d674edafdb2f6/src/operations/query.ts#L313-L318

@kitten
Copy link
Member

kitten commented Dec 19, 2019

The above PR should fix this! Sorry again for being a little slow on this one

@kitten kitten changed the title Support for @connection, custom cache merging and a real fetchMore Support simplePagination cases as automatic resolver with cache misses supported Dec 19, 2019
@sporieg
Copy link

sporieg commented Dec 23, 2019

Will it fix it?

My problem is the pass|fail changes between running in any repo that uses the npm package, and this repo from github. Adding the schema did not change the results at all. This is still the case in 2.0.0.

If I run the test in my repo with

import { simplePagination } from '@urql/exchange-graphcache/extras';
import { cacheExchange } from '@urql/exchange-graphcache';

It fails, the exact same code with

import { simplePagination} from './extras';
import { cacheExchange } from "./cacheExchange";

From this repo passes.

@JoviDeCroock
Copy link

JoviDeCroock commented Dec 23, 2019

I'll see about getting a publish out later this evening/tomorrow @sporieg

@kitten
Copy link
Member

kitten commented Jan 10, 2020

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question 🙋 A question or usage-issue that won’t need changes
Projects
None yet
4 participants