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

Add Pagination Arguments to Relationship Connections [2.0.0-alpha.2] #279

Closed
litewarp opened this issue Jun 23, 2021 · 7 comments
Closed
Labels
enhancement New feature or request

Comments

@litewarp
Copy link
Contributor

There are no pagination arguments when retrieving node relationships with relationship properties in the 2.0.0 package. If no relationship properties are defined, i.e. no "Connection" types are generated, then the limit/offset arguments are there.

Adding the fields to the type definitions seems simple, but I cannot discern how difficult it would be to reuse the translation logic from the ordinary relationship functions in the createConnectionAndParams function.

Thanks again!

@litewarp litewarp added enhancement New feature or request inbox labels Jun 23, 2021
@danstarns
Copy link
Contributor

Hey, Thank you for the request. We realise there aren't any arguments on the connection, and this is due to the fact we have plans to implement cursor-based pagination into connections, and this is where they would sit. Currently, there is not an alternative or workaround. This issue will be updated with more info in due time.

@litewarp
Copy link
Contributor Author

litewarp commented Jun 23, 2021

Thanks for the response. I'm not sure I understand why there won't be limit/offset arguments on connections. Or to put it differently, why there will be a difference in pagination between relationships with properties versus relationships without properties.

Without the limit/offset functionality there will be no way to provide links to subsequent pages. For example, with relay hasNextPage and endCursors you can fetch the next group of X number but you can't give the user a link to the third page. You need to be able to offset at a predictable number in order to do so.

I can currently do this with regular relationships, but now if I want to add relationship properties, I would have to entirely rethink pagination, when there is nothing intrinsic to adding relationship properties that necessitates a new pagination scheme.

@danstarns
Copy link
Contributor

Thanks for the response. I'm not sure I understand why there won't be limit/offset arguments on connections. Or to put it differently, why there will be a difference in pagination between relationships with properties versus relationships without properties.

Without the limit/offset functionality there will be no way to provide links to subsequent pages. For example, with relay hasNextPage and endCursors you can fetch the next group of X number but you can't give the user a link to the third page. You need to be able to offset at a predictable number in order to do so.

I can currently do this with regular relationships, but now if I want to add relationship properties, I would have to entirely rethink pagination, when there is nothing intrinsic to adding relationship properties that necessitates a new pagination scheme.

With the connections, we aim to integrate with the relay spec. In relay you use cursor-based pagination. Adding the cursor onto connections is something we have not got round to yet though it's in the back of our minds.

@litewarp
Copy link
Contributor Author

litewarp commented Jun 24, 2021

Based on a conversation with @darrellwarde on Discord, the suggestion was just to use the offset/limit options to accomplish what I need.

Is there any reason that you're pursuing cursor-based pagination over the already-implemented (despite missing aggregations) page-based pagination?

But that's not possible if I want to add relationship properties! And it seems like the only reason we can't get limit / offset on connections is a design choice and not related to the additional functionality (the ability to define and return relationship properties).

I suppose my question is why can't we have both?

@litewarp
Copy link
Contributor Author

Fixed in #282

@darrellwarde
Copy link
Contributor

Morning! Back from a week of leave and want to chime in on this as I understand your frustration and want to give a bit more background.

This was discussed a lot whilst designing the relationship properties work. As you know, relationship fields previously jumped directly to neighbouring nodes, and allow for page-based pagination. We then implemented the "shape" of cursor connections to facilitate relationship properties, but never promised to be a Relay-compliant API at this stage. As you've come up against, this will leave a gap - what if you want to use page-based pagination with relationship properties?

We discussed introducing an "intermediate" field, like moviesRelationships (for a movies example), which would skip the top-level "connection" level and return an array of "edges", but would allow for page-based pagination instead of cursor-based pagination. However, this is where we made a design decision in that this would mean three relationship fields per relationship, which just feels a bit insane. If there is a call for it though, we would be happy to reconsider it - perhaps we need to make which fields are included configurable. Just to put what this could look like into context, here are some rough schema snippets:

type MovieActorsRelationship implements ActedIn {
  node: Actor!
  screenTime: Int!
}

type MovieActorsConnection {
  edges: [MovieActorsRelationship!]!
}

type Movie {
  title: String!
  actors(where: ActorWhere, options: ActorOptions): [Actor]!
  actorsRelationships(where: ActorWhere, options: ActorOptions): [MovieActorsRelationship]!
  actorsConnection(where: MovieActorsConnectionWhere, options: MovieActorsConnectionOptions): MovieActorsConnection!
}

As you can see, it would make it pretty busy, but it would allow you to "jump in" at any "stage" of the relationship. The where and options types would need to be a bit different, but serve the purpose for this example.

As @danstarns has alluded to, we haven't just left cursor-based pagination out, but planned on leaving it for now as we have some ideas on how we want to achieve it and it's set to be another significant piece of work. I'll happily review your PR and it would be great if we can work together towards a solution for the time-being, but please be ready for some back and forth on the solution.

@darrellwarde
Copy link
Contributor

Following the merge of #282, closing, as this feature should now be good for 2.0.0! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants