PageInfo implemented incorrectly?

[adamni21]

Summary

PageInfo is not implement as per relay specification .

Steps to reproduce

I've created a branch, with the cursor related tests adjusted. https://github.com/adamni21/crystal/tree/fix-PageInfo-tests

Note: The adjusted tests still don't quite line up with the specification I think. Since the spec states: "that elements exist prior to after" / "that elements exist following before" which to me (non-native speaker) implies prior/following exclusive after/before, which imho is illogical, and should be inclusive. (the GitHub graphql api agrees with me)

Expected results

The adjusted tests should pass.

Actual results

The adjusted tests do fail.

Possible Solution

🤷

[benjie]

If the client is paginating with first/after, then the client may return true if edges prior to after exist, if it can do so efficiently, otherwise may return false.

When pagination with first/after we cannot efficiently determine if edges prior to after exist, to do so would require an additional query. Hence we follow the “otherwise”, as per the specification, and always return false. I have previously discussed this with members of the relay team and the behaviour we have implemented is correct.

Unless I misunderstand the issue?

[adamni21]

I kind of just ignored the "efficiently determine" part in the specification 🙈

I would however argue, if the client selects hasPreviousPage, it should be okay with the associated perfomance penalty. Otherwise it shouldn't have selected hasPreviousPage in the first place. I mean, the with the current implementation, there is no need to select it for "first" queries, anyway. Because hasPreviousPage will always be false for these.

But if you disagree, we (the company I work at) really need it to work as described by these tests, but I guess it should be achievable via a plugin, or is it tied too closely to internal steps like PgSelect, etc.?

[benjie]

Are you sure you need it to work? Typically you can infer: “if there is an after cursor, then there must be a previous page”, and when you then start paginating backwards hasPreviousPage will be honoured as required by the spec.

I’m frustrated that they didn’t allow null in the case where you opt out due to performance, that would have been more sensible.

Yes you can override the plan for that field with a plugin, but implementing a check to see if there is a previous page is somewhat complex, which is one of the reasons we dropped it from V5.

[adamni21]

Are you sure you need it to work? Typically you can infer: “if there is an after cursor, then there must be a previous page”, and when you then start paginating backwards hasPreviousPage will be honoured as required by the spec.

Yeah, I thought of this alternative as well, I'll discuss it with my team. Thanks for the suggestion!

I’m frustrated that they didn’t allow null in the case where you opt out due to performance, that would have been more sensible.

That would have made more sense, imho 😄

Would it make sense though, to add this to the list of breaking changes, in the v5 docs? Otherwise, I suppose, this ticket can be closed. Thanks again, for your help.

[benjie]

If it’s not already in there then yes that would be a good note to add, I think we did mention changes around nullability for it, so maybe near there? Use your judgement.

[benjie]

Documentation added in #2140