[RFC] Changes to connections spec and implementation

[migueloller]

This will be a long one, please bear with me 😅 .

All ideas here are mostly relevant to pagination, and thus only the connections part of the Relay spec.

After much thought, going over #41, #58, #91, connection.js & the connection spec, and implementing the spec's algorithms for RethinkDB. I've come to a couple of ideas that I would love to hear some comments on (both from the community and the spec and GraphQL.js implementors).

If the ideas expressed here are of interest, I'd be honored to submit a PR.

Let's start with the simple ones:

Spec:

• The spec doesn't require that the cursor field be non-null for the edge type. Should every node have a cursor? It would make sense that if a node exists then there is a unique opaque identifier that can serve as a cursor. Which brings me to the next thought...
• In node type should not be restricted to ObjectType #41 @OlegIlyenko brought up the idea of nodeType being always nullable in the implementation. Would it make more sense for the node field in edges to be non-null?
• In the same vein as the 2 previous thoughts, would it make more sense that the edges field of a connection be a non-null list of non-null edges (currently it is a list of edges)? pageInfo is required and its fields hasNextPage and hasPreviousPage are required as well but these lose all meaning if the edges field can be null.
• Finally (regarding the spec), I propose a few changes to the pagination algorithms in the spec to support bidirectional pagination as discussed in pageInfo.hasPreviousPage is always false if you are paging forwards #58. The algorithm is described at the end of this post.

Implementation:

• Implement any or all of the changes mentioned above.
• Currently first and last are validated in arrayconnection.js (i.e., in the implementation of the paging algorithm). Would it make more sense to have this validation happen at the schema level (what I think the spec intends) by implementing an UnsignedInt scalar and using that as arguments in forwardConnectionArgs, backwardsConnectionArgs and connectionArgs?
• Would it be useful to implement a function that validates a schema by making sure that the cursor type used to query connections is the same as the cursor type that the connection's edges has?
• The spec notes that a cursor can be any scalar that serializes to a string. Currently, this implementation forces cursors to be a string. Expanding on Run PageInfo.startCursor and endCursor through resolveCursor() ? #91, would it make sense to add a cursorType parameter to connectionDefinitions that would then use the given cursor type instead of a GraphQLString? forwardConnectionArgs, backwardsConnectionArgs and connectionArgs could then be returned from connectionDefinitions with the appropriate cursor type (removing the need of the previous thought of adding a schema validator... as long as the developer does use the returned objects).

And that's it! Again, I will most likely be implementing most of these for our backend so I wouldn't mind at all to submit a PR. Also, I do understand that these might not be useful to others but I thought they could be so I wanted to hear people's thoughts.

Thanks for reading this huge wall of text. Please comment, criticize, and ask away! 🍺

Pagination Algorithm

... finally, here's the updated pagination algorithm. It should prove trivial to implement with any database as long as there is a way to get the count of records (I successfully implemented it in RethinkDB and can post that source code here if people want to see it). This would solve #58.

Arguments: allEdges, after, before, first, last

1. Initialize edges to be allEdges.
2. Let count be the number of elements in edges.
3. Let afterIndex be the index of the edge in edges whose cursor is equal to the after argument or -1 if not found.
4. If afterIndex is not -1:
   • Remove all elements of edges before and including afterIndex.
5. Let beforeIndex be the index of the edge in edges whose cursor is equal to the before argument or -1 if not found.
6. If before is set:
   • Remove all elements of edges after and including beforeIndex.
7. Let beforeCount be the number of elements in edges.
8. If first is set:
   • If first is less than 0: (this check wouldn't be necessary if we ensure first is non-negative)
     • Throw an error.
   • If edges has length greater than than first:
     • Slice edges to be of length first by removing edges from the end of edges.
9. Let actualFirst be the number of elements in edges.
10. If last is set:
    • If last is less than 0: (this check wouldn't be necessary if we ensure first is non-negative)
      • Throw an error.
    • If edges has length greater than than last:
      • Slice edges to be of length last by removing edges from the start of edges.
11. Let actualLast be the number of elements in edges.
12. If afterIndex is not -1:
    • Let hasPreviousPage be true.
13. If afterIndex is -1:
    • If beforeIndex is 0:
      Let hasPreviousPage be false.
    • If beforeIndex is not 0:
      • If actualFirst is greater than actualLast:
      • Let hasPreviousPage be true.
    • If actualFirst is less than or equal to actualLast:
      • Let hasPreviousPage be false.
14. If afterIndex is equal to count - 1:
    • Let hasNextPage be false.
15. If afterIndex is not equal to count - 1.
    • If beforeIndex is not -1:
      • Let hasNextPage be true.
    • If beforeIndex is -1:
      • If actualFirst is less than beforeCount:
      • Let hasNextPage be true.
    • If actual first is greater than or equal to beforeCount:
      • Let hasNextPage to be false.

Returns: edges, hasPreviousPage, and hasNextPage.

There might be a more succinct way to write the algorithm but I'm 100% sure this works and I used the one from the spec as a baseline.

[Xuhao]

Any update on this?

[rwe]

Regarding the non-nullability suggestions, that would mean that a error resolving the node within any edge in a connection would always propagate to invalidate the entire connection, and that there would be no flexibility on the implementing side to relax that behaviour while still conforming.

As far as I know, right now nothing prevents a schema from asserting non-nullability for those fields on a particular edge type, if it wants or needs to. But to enforce that at the spec level for all edge types seems particularly restrictive in a very brittle way.

[migueloller]

@erydo, that's a very good point! I do agree that having the errors invalidate the entire connection is undesirable. Perhaps the non-nullability suggestions for nodes or edges aren't very good. 😅

I'm sure there would be a benefit to some of the non-nullability constraints as long as it doesn't force a node error to invalidate the entire connection.