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

pageInfo.hasPreviousPage is always false if you are paging forwards #58

Open
gallagher-stem opened this Issue Jan 7, 2016 · 14 comments

Comments

Projects
None yet
@gallagher-stem

gallagher-stem commented Jan 7, 2016

hasPreviousPage: last != null ? startOffset > lowerBound : false,

If my connection args don't have the 'last' parameter, then hasPreviousPage will always be false. Same problem if you are paging backwards and don't have the 'first' parameter in your connection args: hasNextPage will always be false.

But I can't have a 'last' parameter if I am paging forward using 'first' and 'after'. And I can't have a 'first' parameter if I am paging backwards using 'last' and 'before'. Babel-relay-plugin will throw an error on transpile.

So if I am paging forwards, I will always be told I have no previous pages, even when I do. And if I am paging backwards I will always be told I have no next pages, even when I do.

This has gotta be a bug. It kinda ruins bi-directional paging.

Can't we just make paging easier and let us pass first and last and before and after all as connection args (some of them as null depending on which way you are paging) without babel-relay-plugin blowing up?

@MattMcFarland

This comment has been minimized.

MattMcFarland commented Jan 7, 2016

I know this seems counter-intuitive to you as it did to me as well, but that is actually how it is supposed to work...

hasPreviousPage is only meaningful when last is included, as it is always false otherwise. hasNextPage is only meaningful when first is included, as it is always false otherwise. When both first and last are included, both of the fields are set according to the above algorithms, but their meaning as it relates to pagination becomes unclear. This is among the reasons that pagination with both first and last is discouraged.

https://facebook.github.io/relay/graphql/connections.htm#sec-undefined.PageInfo.Fields

Sorry I couldn't use markdown in the post I emailed.
I also posted about this, was embarrassing hehe.
#55

@gallagher-stem

This comment has been minimized.

gallagher-stem commented Jan 7, 2016

Thanks for the response, Matt.

So its the actual Relay spec that has a bug. :(

In any case, it makes no sense, and ruins bi-directional paging.

Since the maintainers of this repo are the same guys maintaining the spec, I'm gonna leave this open, as it really needs to be addressed.

@dschafer

This comment has been minimized.

Contributor

dschafer commented Jan 7, 2016

Yeah, this is good feedback.

We went with this approach because it's easy to envision a backend system (and we had some at Facebook) where it was be prohibitively expensive to compute hasPreviousPage when paginating forward (or vice versa). The cursors that we were using for pagination made it very efficient to skip straight to the item in question, but trying to traverse backwards to see if there was anything before it would have been costly. Hence, we didn't assign meaning to hasPreviousPage, because we had no way to provide an accurate value there.

Your feedback is spot on, though; this definitely messes with bidirectional pagination. One way forward would be to modify the Relay specification and allow, though not require, hasPreviousPage to have the expected meaning. Connections where hasPreviousPage is easy to compute can then include it, where cases where it is expensive can continue returning false. Clients can then enable the bidirectional pagination behavior you're looking to add only if they ever see hasPreviousPage be true when paginating forward.

Thoughts?

@gallagher-stem

This comment has been minimized.

gallagher-stem commented Jan 7, 2016

Thanks for the reply, Dan. Loving the Relay.

tl;dr: connectionFromArray should assume that its got an entire dataset, so it can be honest about hasNextPage/hasPreviousPage. connectionFromArraySlice should assume that its got only part of a larger data set, so it cant be honest about hasPrevious page when its going forward (or vice-versa).

At the end of the day, to make good on the Relay goal of making data fetching/caching as easy and opaque as possible (which is an awesome goal), paging just "needs to work". And that means supporting the commonly understood ideas of what paging in apps looks like, which is pretty much either infinite scroll (forward-only) or next/prev (bi-directional) with/without page numbers.

We can check off forward-only because that works great! ;)

I was messing with arrayconnection.js and it was pretty simple to stop taking last/first into account when calculating and comparing the array bounds and the current cursor location. Relay on the client thwarted me, however, because it still overrides the values for hasNextPage/hasPreviousPage on the client based on whether first/last is being used.

I guess I don't understand why connectionFromArray can't give back honest values from hasNextPage/hasPreviousPage. Its assumed that we are passing in the whole list of data as the array. Why not be honest with the hasNextPage/hasPreviousPage?

connectionFromArraySlice can and should be used when we are using forward-only paging on large data-sets. The comments in the code come out and say as much. It kinda looks like we just found an easy way to have connectionFromArray delegate its work to connectionFromArraySlice. And connectionFromArraySlice correctly assumes it cant honestly answer if there is a previous page when going forward on what it has been told is just a slice of a larger array.

Maybe some renaming would make things clearer. connectionFromArray becomes bidirectionalPagingConnection (or entireDataSetConnection or something) and connectionFromArraySlice becomes forwardOnlyPagingConnection (or partialDataSetConnection or something).

And then change connectionArgs validation. In fact, Im not convinced we need both 'first' and 'last' if we can just make some assumptions. Instead of first/last we just have 'count', because thats what both those are: just a count of records to return. Then if we have count without before or after, we assume its going forward from the start (which I think is the 99%+ use case). If we want to start out going backward from the end, you pass a 'before: end' (or before: '' or something). Other than that, before and after cursors work the same, and args validation just changes to not allow both before and after together.

And then change the Relay spec to reflect these changes. ;)

(And then figure out page numbers, which once we decide that connectionFromArray is meant for entire datasets, is an easy prop to add to pageInfo and connectionArgs).

To maybe help out anyone currently wrangling with this same issue, here is a link to a gist that shows how I am hacking together next/prev paging in Relay. It was heavily informed by this fine gentleman's efforts with the same problem.

Anyway, thanks for the listen. I'm having so much fun getting into Relay and I love what it does and what its going to do. Rock on, rockstars!

@artyomtrityak

This comment has been minimized.

artyomtrityak commented Apr 5, 2016

I am trying to get hasNextPage with connectionFromPromisedArray but it does not work (returns false). first: 5 i am passing, there are more rows in database. It seems like it will work only with connectionFromArraySlice because graphql-relay will get all data set and slice it. I read spec but still do not understand how it expected to work

@bruce

This comment has been minimized.

bruce commented Apr 18, 2016

Ugh, this issue is so deeply annoying when having a hard requirement to do windowed pagination due to limited space. Even with a server returning hasPreviousPage as true (because it can actually calculate that), hasPreviousPage gets passed on to components as false. Are there any decent examples of anyone doing a workaround that supports windowed pagination? I could care less about jumping to pages, but accurate previous & next links is vital.

@MattMcFarland

This comment has been minimized.

MattMcFarland commented Apr 18, 2016

The pageType and hasNextPage/hasPrevPage is declared in connection.js, but the logic appears to be in arrayconnection.js

export function connectionFromArraySlice<T>(
arraySlice: Array<T>,
args: ConnectionArguments,
meta: ArraySliceMetaInfo
): Connection<T> {
var {after, before, first, last} = args;
var {sliceStart, arrayLength} = meta;
var sliceEnd = sliceStart + arraySlice.length;
var beforeOffset = getOffsetWithDefault(before, arrayLength);
var afterOffset = getOffsetWithDefault(after, -1);
var startOffset = Math.max(
sliceStart - 1,
afterOffset,
-1
) + 1;
var endOffset = Math.min(
sliceEnd,
beforeOffset,
arrayLength
);
if (typeof first === 'number') {
endOffset = Math.min(
endOffset,
startOffset + first
);
}
if (typeof last === 'number') {
startOffset = Math.max(
startOffset,
endOffset - last
);
}
// If supplied slice is too large, trim it down before mapping over it.
var slice = arraySlice.slice(
Math.max(startOffset - sliceStart, 0),
arraySlice.length - (sliceEnd - endOffset)
);
var edges = slice.map((value, index) => ({
cursor: offsetToCursor(startOffset + index),
node: value,
}));
var firstEdge = edges[0];
var lastEdge = edges[edges.length - 1];
var lowerBound = after ? (afterOffset + 1) : 0;
var upperBound = before ? beforeOffset : arrayLength;
return {
edges,
pageInfo: {
startCursor: firstEdge ? firstEdge.cursor : null,
endCursor: lastEdge ? lastEdge.cursor : null,
hasPreviousPage:
typeof last === 'number' ? startOffset > lowerBound : false,
hasNextPage:
typeof first === 'number' ? endOffset < upperBound : false,
},
};
}

This code here looks suspect(startOffset, endOffset, lowerBound, and upperBound):

  return {
    edges,
    pageInfo: {
      startCursor: firstEdge ? firstEdge.cursor : null,
      endCursor: lastEdge ? lastEdge.cursor : null,
      hasPreviousPage:
        typeof last === 'number' ? startOffset > lowerBound : false,
      hasNextPage:
        typeof first === 'number' ? endOffset < upperBound : false,
    },
  };

Specifically , the startOffset and endOffset are not set unless you use "first" or "last":
The source code below is found in the same file, slightly above.

  if (typeof first === 'number') {
    endOffset = Math.min(
      endOffset,
      startOffset + first
    );
  }
  if (typeof last === 'number') {
    startOffset = Math.max(
      startOffset,
      endOffset - last
    );
  }

The upperBound and lowerBound are set like so:

  var lowerBound = after ? (afterOffset + 1) : 0;
  var upperBound = before ? beforeOffset : arrayLength;

Looks like you need to set after, before, first, and last and you will get dual pagination to work. I haven't tested but looking at the source code I can't see why not!

@staylor

This comment has been minimized.

staylor commented Nov 30, 2016

This is really annoying - I had to implement my own methods, even though I can generate these values properly in the GraphQL response:

const idxPrefix = 'idx---';

const fromBase64 = (encoded: string): string => 
  Buffer.from(encoded, 'base64').toString('utf8');

const indexFromCursor = (cursor: string): number =>
  parseInt(fromBase64(cursor).replace(idxPrefix, ''), 10);

const hasPreviousPage = (startCursor: string): boolean => 
  indexFromCursor(startCursor) > 0;

const hasNextPage = (endCursor: string): boolean => 
  (indexFromCursor(endCursor) + 1) % limit === 0;

I have a feeling that this will be a continued problem, or people will choose to not adopt Connections / Edges, and instead just paginate via hints from parameters in a Route.

@thomasloh

This comment has been minimized.

thomasloh commented Jan 20, 2017

The other workaround is to put state of graphql query in the url

Page 1
/list
Page 2
/list?after={endCursor1}
Page 3
/list?after={endCursor2}
Page 4
/list?after={endCursor3}

Going forward works well as it is, to go backward simply go back in history, i.e. window.history.go(-1)

If ?after= is not present, Previous button can be hidden

@ivosabev

This comment has been minimized.

ivosabev commented Feb 6, 2017

Any status on resolving this?

@wincent

This comment has been minimized.

Contributor

wincent commented Feb 14, 2017

@ivosabev: I'm not aware of anybody actively working on this, but given Dan's comment (quoting below) I think it would be reasonable to accept a PR that modified the spec:

Your feedback is spot on, though; this definitely messes with bidirectional pagination. One way forward would be to modify the Relay specification and allow, though not require, hasPreviousPage to have the expected meaning. Connections where hasPreviousPage is easy to compute can then include it, where cases where it is expensive can continue returning false. Clients can then enable the bidirectional pagination behavior you're looking to add only if they ever see hasPreviousPage be true when paginating forward.

@du5rte

This comment has been minimized.

du5rte commented May 31, 2017

Any update or work around on this issue? @gallagher-stem comment seems like a good solution,

@eapache

This comment has been minimized.

eapache commented Sep 1, 2017

I have PRed a change to the spec to permit setting these values properly so that at least other implementations can do so without breaking technical compliance: facebook/relay#2079

facebook-github-bot added a commit to facebook/relay that referenced this issue Sep 12, 2017

Permit bidirectional PageInfo
Summary:
The existing specification for PageInfo requires that `hasPreviousPage`
be false when paginating forward, which is quite limiting. Adjust that
part of the specification to permit setting `hasPreviousPage` when
paginating forward, and symmetrically permit setting `hasNextPage` when
paginating backward. Implementations may still hard-code false if
calculating the correct value would be inefficient. This was suggested
by dschafer in graphql/graphql-relay-js#58 (comment).

Making this change exposed an inconsistency in handling unusual cases
such as when a client provides only `last` and `after` arguments. I
adjusted a few other parts of the algorithm to clarify how
best to handle these cases.

cc swalkinshaw
Closes #2079

Differential Revision: D5811407

Pulled By: leebyron

fbshipit-source-id: 431335dd07cb5c2536d6bfdb8fc1b4a7e4900828
@richardscarrott

This comment has been minimized.

richardscarrott commented Oct 17, 2018

Are PRs for this still welcome or should this issue be closed as won't fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment