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

Adjusting the store in order to squash pagination results via Relay's @connection directive. #1779

Closed
lxwbr opened this issue Jun 9, 2017 · 10 comments · Fixed by #1801
Closed

Comments

@lxwbr
Copy link

lxwbr commented Jun 9, 2017

I am using the Relay convention for pagination as described here. Coming from Relay Modern, there is a directive called @connection. It contains a key which Relay makes use of in order to retrieve all elements of one connection from the store. Apollo Client on the other side does ignore the @connection and stores objects by using keys containing variables like after or count, used to fetch the right amount of data by using a cursor based pagination approach.

The real problem of that comes into play when I am executing a mutation for adding a new edge into the connection. Here the result has to be stored under the correct key in the store via proxy.readQuery(...), which is impossible if you have multiple keys for lists of the same connection.

I think the problem is better understandable with a short example. This is how a proxy store should look like and how it is right now:

Intended outcome:

Project:AAAAAAAAA
  | issues -> ref to $Project:AAAAAAAAA.issues.edges

$Project:AAAAAAAAA.issues.edges
  | edges
    | 0 -> ref to $Project:AAAAAAAAA.issues.edges.0
    | ...

$Project:AAAAAAAAA.issues.edges.0
$Project:AAAAAAAAA.issues.edges.1
$Project:AAAAAAAAA.issues.edges.2
$Project:AAAAAAAAA.issues.edges.pageInfo

Actual outcome:

Project:AAAAAAAAA
  | issues({"first":2,"after":null}) -> ref to $Project:AAAAAAAAA.issues({"first":2,"after":null})
  | issues({"first":1,"after":XYXYXYXYXY}) -> ref to $Project:AAAAAAAAA.issues({"first":1,"after":XYXYXYXYXY})


$Project:AAAAAAAAA.issues({"first":2,"after":null})
  | edges
    | 0 -> ref to $Project:AAAAAAAAA.issues({"first":2,"after":null}).edges.0
    | 1 -> ref to $Project:AAAAAAAAA.issues({"first":2,"after":null}).edges.1
$Project:AAAAAAAAA.issues({"first":1,"after":XYXYXYXYXY})
  | edges
    | 0 -> ref to $Project:AAAAAAAAA.issues({"first":1,"after":XYXYXYXYXY}).edges.0

$Project:AAAAAAAAA.issues({"first":2,"after":null}).edges.0
$Project:AAAAAAAAA.issues({"first":2,"after":null}).edges.1
  | cursor="XYXYXYXYXY"
$Project:AAAAAAAAA.issues({"first":2,"after":null}).edges.pageInfo

$Project:AAAAAAAAA.issues({"first":1,"after":XYXYXYXYXY}).edges.0

How can I tell the Apollo Client store to use a unified key for storing edges of the same connection, ignoring fields like after or cursor used for accessing the field? Is there a way to make use of the @connection information provided in the schema?

@jonmanzo
Copy link
Contributor

jonmanzo commented Jun 12, 2017

You won't need to worry about that provided you are using fetchMore -> updateQuery to update your cursor or in your case first/after. The query will get stored in redux with the new key name based on the variables, but the data will flow down to the original query. That said, the readQuery/writeQuery is the spot that needs some work. This is what I did (in an infinite scroll solution...):

I prefetch immediately after login as to instantiate cache for multiple queries:

await this.props.client.query({ query: ALL_ITEMS_QUERY, variables: rootQueryVariables('allItems'), });

Then the Key to it working....

The below is within my mutate.update function. unshiftItemToQuery is simply a helper but the point to note is that "rootQueryVariables" will always return the same vars as the initial "allItems" query.

// All Items unshiftItemToQuery( proxy, { mutationResult, query: ALL_ITEMS_QUERY, variables: rootQueryVariables('allItems'), ownProps, which: 'allItems', self: 'assignItem', })

Since the paginated data flows down to the initial query, reading the query with the initial vars will allow you to find the result set and mutate that.

@lxwbr
Copy link
Author

lxwbr commented Jun 12, 2017

@jonmanzo That variables part is exactly what gives me headache. I guess you're having some sort of store for variables accessible through the rootQueryVariables function. I imagine it being a bit problematic if the variables are given through user input. Surely there are ways to implement rootQueryVariables to hack around it, but this solution still feels like a hack, as the original Apollo store implementation does not provide the solution for that case.

It is also worth mentioning that the work-around causes the problem of having links to paginated objects twice in the data store. Though in most cases it shouldn't be a problem, it still feels very wrong to have this redundancy in the store.

In my opinion there is need to access the fields in store by some unique id, which can be defined on the query level. Relay has a pretty nice way of doing it by @connection annotation:

query PeopleQuery($count: Int!, $cursor: String) {
   persons(first: $count, after: $cursor) @connection(key: "KEY_TO_RETRIEVE_PERSONS") {
      ...
   }
}

and then accessing that connection from store via ConnectionHandler.

PS: for now I am using another work around of updateQueries in the mutate function. Which I am still not comfortable with, as it is relying on the exact naming of the initial query. Additionally I have to drill down the whole query tree in order to push into the affected array instead of directly accessing it from the store. (in contrary to e.g. proxy.readFragment(...) if that one would accept my proposal of @connection key directive)

@jonmanzo
Copy link
Contributor

Agreed 100%, it is a hack in my opinion and I've had to make functional exclusions to work under this model to boot. Digging through the src, I didn't find any other options that would allow a modification of the key utilized to store the query...

Honestly, I'd even be ok with the ability to define a query cache key name. We're pushing an ES query through the args, thus the potential exists for an insane amount of possible argument structures. Allowing the ability to choose what arguments are utilized in the key name would clean this up instantly in our use case.

@jonmanzo
Copy link
Contributor

Oh, and we cannot solely use update queries as we're moving objs from one cache to another based in user action and ran into issues where the destination query was no longer watched once their component unmounted. Found this a better solution that putting all the queries into a top level wrapper just to keep them available to updateQueries

@stubailo
Copy link
Contributor

We did want to have this in the original implementation for pagination, but didn't do it at first because the simpler version has worked fine for a while. I agree that being able to pick a custom store field name is a great way to do this.

We used to call this concept quietArguments and were thinking of an @apolloFetchMore directive: #350

Perhaps we should bring this back and now may be the time to do it!

@helfer
Copy link
Contributor

helfer commented Jun 14, 2017

@Zunder As @stubailo said, we think it would be a good idea to allow for an @connection directive. The key would determine where in the store the connection gets written to, so it would be easy to read it out with readQuery. The only thing that we'd have to decide is how you would tell Apollo Client when you're fetching more for a certain connection, and whether that "more" goes at the beginning, the end or somewhere in the middle.

@stubailo
Copy link
Contributor

I don't think we need to change anything about the fetchMore mechanism to add this directive - we could still use the manual reducer approach. Only difference would be having a more reasonable cache key!

@helfer
Copy link
Contributor

helfer commented Jun 15, 2017

Yeah, you're right, since fetchMore is imperative we don't need any configuration.

@lxwbr
Copy link
Author

lxwbr commented Jun 29, 2017

@helfer just tried it out with #1801 and the store structure seems to be as expected in the intended outcome. Thank you and @shadaj for providing this feature!

@jonmanzo
Copy link
Contributor

jonmanzo commented Jul 7, 2017

This is great, I second the thanks! 👯

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants