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

pagination with previous/next #466

Closed
faassen opened this issue Oct 14, 2015 · 8 comments
Closed

pagination with previous/next #466

faassen opened this issue Oct 14, 2015 · 8 comments
Assignees
Labels

Comments

@faassen
Copy link

faassen commented Oct 14, 2015

It seems to me that implementing pagination through a connection with both a previous and a next button is currently rather difficult to implement. (I want the previous/next buttons to be hidden or disabled when there's no previous or next page). I finallly managed to implement it, it's pretty hairy:

  • am I making things too complicated? I'm a noob. But in that case there needs to be documentation or an example that shows the easy way.
  • is this really a supported use case? If not, the documentation needs to make that very clear. You'd think it is a supported use case, though.

My initial attempt was to define a query along these lines:

stories(first: $first after: $after last: $last before: $before)

The idea was to set the variables to something like this when clicking previous:

first: null,
after: null,
last: 10,
before: stories.pageInfo.startCursor

and when going next, do this:

last: null,
before: null,
first: 10,
after: stories.pageInfo.endCursor

(incidentally, pageInfo.startCursor and pageInfo.endCursor don't appear here: https://facebook.github.io/relay/graphql/connections.htm Should they?)

This failed because the compiler complains when I include those variables at the same time.

So then I split up the code to please the compiler:

previousStories: stories(last: $last before: $before) @include(if: $wantPrevious) {
}
nextStories: stories(first: $first after: $after) @include(if: $wantNext) {
}

Note that I noticed that Relay when you pass in a variable of null in these cases does omit the variables, so is the compiler currently too strict?

In the React code I then have to decide whether to display previousStories or nextStories depending on whether there is any content there.

Now I can paginate previously and next. I do get a ton of warnings I have no clue what to do about:

GraphQLRange currently only handles first(<count>), after(<cursor>).first(<count>), last(<count>), before(<cursor>).last(<count>), before(<cursor>).first(<count>), and after(<cursor>).last(<count>)

This may be related to #247, but it's a different warning...

I'm incidentally confused about the behavior of @include as even when $wantPrevious is false I still get a full-fledged 'previousStories' object, and I have to check edges.length to see whether it's really empty. I guess @include only applies to that field and other fields are loaded but empty? It's unclear to me what it's supposed to do from the GraphQl spec...

Now I get to the next problem: the previous button shouldn't do anything (or be greyed out or hidden) if you are at the start of the content, and the next button shouldn't do anything (or be greyed out or hidden) if you're at the end. But how does one accomplish this? At first sight you'd think hasPreviousPage and hasNextPage in pageInfo can do it, but they can't, as hasPreviousPage is always false if you're paginating forward and vice versa. That's not what is needed for this use case, but it is at the very least rather misleading. Here are these fields that given their names you think are perfect for this use case, but it's a lot more subtle than that.

So I came up with something like this:

previousStory: stories(last: 1 before: $before) {
}
nextStory: stories(first: 1 after: $after) {
}

but I was defeated again: previousStory gives back 1 story, even if I'm at the start of the array: the last item! This is because I actually pass in $before as null in the initial query, which means no before parameter is passed in at all, and this gives the last entry. The same problem occurs for nextStory.

So back to pageInfo again. Can I somehow get the information I want out of it after all? It turns out I can, but it's hairy:

        if (this.props.relay.variables.wantPrevious) {
            hasPrevious = this.props.viewer.previousStories.pageInfo.hasPreviousPage;
            hasNext = true;
        } else {
            // we want the next page
            // if the after variable is null, we don't have previous
            // otherwise, we always do as we just got the next batch
            hasPrevious = this.props.relay.variables.after !== null;
            hasNext = this.props.viewer.nextStories.pageInfo.hasNextPage;
        }

so, if we just retrieved the previous page (wantPrevious), we know that hasPreviousPage has useful information, and we know there must be a next batch as we just came from there.

if we just retrieved the next page, then we know that hasNextPage has useful information, and we know
that there must be a previous page, unless we're at the first page which we can detect by checking whether the after variable is the initial null, as at the first page we never have a previous page anyway.

This appears to work, but it requires at least two things that are rather non-obvious:

  • conditionally get previous and next page based on additional wantPrevious/wantNext variables, and logic to check which we got in React component to show the stuff we really did get.
  • hairy logic to determine whether we have a previous and next page.

So in conclusion, again: am I missing something obvious here in which documentation may be able to help)? Or is this an unsupported use case I should've stayed away from? Perhaps a Relay Connection only support expanding web pages with "load more", not a batch display. If so, documentation could point this out - in fact the whole API currently is misleading with its 'hasNextPage' naming too. Perhaps for next/previous batching some other connection-like facility (with limit/offset?) would be more useful.

@josephsavona josephsavona self-assigned this Oct 14, 2015
@josephsavona
Copy link
Contributor

@faassen Thanks for the question! You're definitely not missing something: Relay was designed to make infinite scroll easy and hence the API for this is relatively simple. However, windowed pagination hasn't been a common requirement so it currently requires more work (as you've seen). That said, we're open to suggestions and contributions to make this use-case simpler for developers.

Do you need to be able to jump to arbitrary pages in the list (e.g. would you have links to non-adjacent pages such as "...[2] [3] [4] ... [10]"), or would the user always page through from the beginning (via previous/next links)?

@gregkaczan
Copy link

@josephsavona Although not entirely connected with initial question above i jumped in because i see this going in the direction i need. So im adding my question.

What you asked:
would you have links to non-adjacent pages such as "...[2] [3] [4] ... [10]

is exactly what i need.
Our current graphql server is extended so that it can send back total count along with native pageInfo information. So i have pageInfo.total but Relay does not know it and completely ignores.

How can i pass it nicely to the component (i need total count to be able to render page numbers)?

@josephsavona
Copy link
Contributor

@GrzegorzKaczan Instead of putting the count on pageInfo, make it a field on the connection. E.g.

type FooConnection {
  total: Int
  edges: [FooEdge]
  pageInfo: ...
}

@gregkaczan
Copy link

ha, thought so, thx.
Awesome stuff btw, hail Relay+GraphQL!

@faassen
Copy link
Author

faassen commented Oct 19, 2015

This was is not driven by any concrete need, just my experimentation
with Relay. I was thinking about writing a Relay tutorial, but we'll
see. I'll get back to this in the end of my comment.

I wanted to report on my adventures first to give an impression of the
struggles of a beginner, but will try to distill this to some concrete
questions/suggestions.

My main point is that it would be better if either:

  • The server clearly separates between a forward and backwards
    connections, with two connection types. The Relay JS API on the
    server could indicate which type with a flag for convenience. The
    current situation is almost that due to the behavior of the
    compiler, but it's more misleading than it has to be.
  • Relay fully supports bidirectional navigation for connection.

This may mean you actually want both: a forward type, a backward type and a
bidirectional type.

Now into the details:

  • Hitting on the idea of using an @include(if to make this work is
    quite involved:

    • you need to realize @include(if is needed to solve this at all.
    • you need to create an extra variable to signal with a flag what
      branch you want to take. I cannot rely on the state of after and
      before because they start out with null in the first page.
    • you need to add additional logic to determine whether you got
      information in previousStories or nextStories (or use the
      state of the flag described above) and then return this information.
    • to enable/disable the previous and next buttons appropriately
      you also need to be aware of the state of the flag, and in additional you
      need to know whether you're on the first page. Far from obvious.

    If there were explicit unidirectional connection types then this
    solution would become more obvious.

  • If you want to truly support bidirectional navigation then I think
    you need this:

    • not have the compiler forbid passing in all parameters at once. I believe
      the compiler only got more strict recently, but the previous
      behavior of runtime checks for impossible combinations is better for
      this use case (if there is a first then last should be null).
    • both pageInfo.hasPreviousPage and pageInfo.hasNextPage should
      always return useful values. The current naming invites you to
      think it will work, and only digging in the docs leads to find out
      that one of the two will always be false depending in which
      direction you are going.
  • The GraphQLRange currently only handles first(<count>), after(<cursor>).first(<count>), last(<count>), before(<cursor>).last(<count>), before(<cursor>).first(<count>), and after(<cursor>).last(<count>) warnings that show up confuse me. I
    don't understand why I get them, or what I should do about them? It
    leads me to think perhaps my current solution even though it seems
    to work actually still has issues I don't understand, but is that
    true?

  • It is suprising that I have to inspect
    previousStories.edges.length to see whether it's empty even though
    I signaled I don't want previousStories with @include(if. I
    expected previousStories to be null instead. I don't understand
    what is going on here.

You may be right in that in a bidirectional UI people generally want
to know something about the total and have the facility to link
directly to individual pages. Though I think forward/backward
pagination without such direct jumping does exist; maybe someone more
familiar with UI requirements can speak up. It may be we need four
connection types:

  • forward
  • backwards
  • bidirectional
  • bidirectional with total and direct page jumps.

@faassen
Copy link
Author

faassen commented Oct 19, 2015

Oh, another potential pagination feature that is needed sometimes is "jump to start" and "jump to end".

@josephsavona
Copy link
Contributor

@faassen thanks again for bringing this up. I've opened a new issue that documents some of the main tasks to better support windowed pagination at #540 - let's continue discussion there now that the requirements are more well defined.

@dminkovsky
Copy link
Contributor

Thanks for posting this @faassen. I prototyped a version of my current app with traditional window-based paging. Realized that's not what Relay is after, and your post and this thread really helped distill all the issues for me. Much appreciated.

I don't have hard requirements for how paging will work, so I will re-work the paging to be Relay style. But if I had a hard requirement from a boss or client, this would be a bummer!

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

No branches or pull requests

4 participants