graphql / graphql/graphql-relay-js

[RFC] Changes to connections spec and implementation

Open
#103 3 comments 4 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
1.5k
Forks
177
Avg merge
10m
Merged PRs (30d)
1

Description

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](https://github.com/graphql/graphql-relay-js/blob/master/src/connection/connection.js) & the [connection spec](https://facebook.github.io/relay/graphql/connections.htm), 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 #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 #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`](https://github.com/graphql/graphql-relay-js/blob/master/src/connection/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](https://facebook.github.io/relay/graphql/connections.htm#sec-Arguments)) 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](https://facebook.github.io/relay/graphql/connections.htm#sec-Cursor) that a cursor can be any scalar that serializes to a string. Currently, this implementation forces cursors to be a string. Expanding on #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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.