Skip to content

Conversation

@wincent
Copy link
Contributor

@wincent wincent commented Oct 8, 2015

The existing connectionFromArray requires an array representing all the records in a connection. Actually materializing such an array may be prohibitively expensive when dealing with large connections (containing thousands of items, or more). This largely defeats the purpose of pagination, which is supposed to free us from having to deal with total data sets when subsets suffice.

At the moment, it is possible to do some ghastly workarounds to trick connectionFromArray into thinking it has a fully materialized array. For example:

const paddedItems = new Array(offset).concat(...items);
if (count > offset + args.first) {
  paddedResults.length++;
}
const connection = connectionFromArray(paddedItems, args);

In other words, we create an array with, say, 1000 empty slots, then insert the 10 items actually retrieved, then increment the length property on the array to trick connectionFromArray into thinking that hasNextPage should be true.

This commit seeks to address this common use case by implementing connectionFromArraySlice. This effectively allows you to fetch the data for a subset of a connection, and pass that in with some metadata indicating where the subsequence starts and how long the total array would be if materialized, and get a connection object back.

We also add connectionFromPromisedArraySlice, for parity with connectionFromPromisedArray, and we reimplement the existing connectionFromArray to call through to the new function.

All specs pass, except one, which I actually think is a bug in the old implementation, so I changed it. The test case uses first: 0 with no before or after. Previously, the code returned hasNextPage: false, but according to the spec, it seems it should actually return true.

HasNextPage is specified as follows:

  1. If first was not set, return false.
  2. Let edges be the result of calling ApplyCursorsToEdges(allEdges, before, after).
  3. If edges contains more than first elements, return true.
  4. Return false.

Given that there are no before or after calls, and ApplyCursorsToEdges specifies that allEdges should be returned in this case, it seems we should be returning true as per step 3 above.

One final thing to note: I've exported cursorToOffset as it can be useful to schema authors when they prepare array slices suitable for passing in to connectionFromArraySlice.

The existing `connectionFromArray` requires an array representing all
the records in a connection. Actually materializing such an array may be
prohibitively expensive when dealing with large connections (containing
thousands of items, or more). This largely defeats the purpose of
pagination, which is supposed to free us from having to deal with total
data sets when subsets suffice.

At the moment, it is possible to do some ghastly workarounds to trick
`connectionFromArray` into thinking it has a fully materialized array.
For example:

```
const paddedItems = new Array(offset).concat(...items);
if (count > offset + args.first) {
  paddedResults.length++;
}
const connection = connectionFromArray(paddedItems, args);
```

In other words, we create an array with, say, 1000 empty slots, then
insert the 10 items actually retrieved, then increment the `length`
property on the array to trick `connectionFromArray` into thinking that
`hasNextPage` should be true.

This commit seeks to address this common use case by implementing
`connectionFromArraySlice`. This effectively allows you to fetch the
data for a subset of a connection, and pass that in with some metadata
indicating where the subsequence starts and how long the total array
would be if materialized, and get a connection object back.

We also add `connectionFromPromisedArraySlice`, for parity with
`connectionFromPromisedArray`, and we reimplement the existing
`connectionFromArray` to call through to the new function.

All specs pass, except one, which I actually think is a bug in the old
implementation, so I changed it. The test case uses `first: 0` with no
`before` or `after`. Previously, the code returned `hasNextPage: false`,
but according to the [spec](), it seems it should actually return
`true`.

[HasNextPage]() is specified as follows:

1. If first was not set, return false.
2. Let edges be the result of calling ApplyCursorsToEdges(allEdges,
   before, after).
3. If edges contains more than first elements, return true.
4. Return false.

Given that there are no `before` or `after` calls, and
[ApplyCursorsToEdges]() specifies that `allEdges` should be returned in
this case, it seems we should be returning `true` as per step 3 above.

One final thing to note: I've exported `cursorToOffset` as it can be
useful to schema authors when they prepare array slices suitable for
passing in to `connectionFromArraySlice`.

[ApplyCursorsToEdges]: https://facebook.github.io/relay/graphql/connections.htm#ApplyCursorsToEdges()
[HasNextPage]: https://facebook.github.io/relay/graphql/connections.htm#HasNextPage()
[spec]: https://facebook.github.io/relay/graphql/connections.htm
I kept these separate from the parent commit for clarity:

- Prefer trailing commas everywhere.
- Sort exported types alphabetically within groups.
- Use object short notation.
- Prefer idiomatic `== null` check over explicit `=== null` and `===
  undefined.
- (Subjective) use a ternary in one place where it doesn't harm
  readability.
@wincent
Copy link
Contributor Author

wincent commented Oct 8, 2015

Adding some Relay peeps: @steveluscher, @yungsters, @josephsavona.

I'll add a separate test for connectionFromPromisedArraySlice which I think should satisfy the coverage complaint.

Add some separate, light tests for `connectionFromArraySlice` and
`connectionFromPromisedArraySlice`, rather than relying on them being
tested indirectly (via `connectionFromArray` and
`connectionFromPromisedArray`).

Additionally, consolidated all the `arrayconnection.js` tests in a
single file.

[1]: At least, I think this will appease them.
- No need to capitalize initial letter of description.
- Jettison implied "Correctly" qualifier.
- Reduce unnecessary nesting of `describe` blocks (ie. siblingless
  nested `describe` blocks).
- Suffix function names with `()`.
Fix things Travis was complaining about, one of which (an early `return`
I'd left behind) was actually masking a real bug.
In building a schema using `connectionFromArraySlice`, I've found it
useful to have access to functionality like `getOffset`. This commit
exposes it as an export, after changing the name to be more descriptive,
do further distinguish it from the existing `cursorToOffset`.

Here's an example of the function in use:

    resolve: async (_, args) => {
      const offset = getOffsetWithDefault(args.after, -1) + 1;
      const [articles, totalCount] = await articleLoader.readIndex(
        args.first, offset
      );
      return connectionFromPromisedArraySlice(
        articleLoader.loadMany(articles),
        args,
        {
          sliceStart: offset,
          arrayLength: totalCount,
        },
      );
    },

Without the function, I end up reimplementing equivalent logic, such as:

    const after = args.after && cursorToOffset(args.after);
    const offset = isNaN(after) ? 0 : after + 1;
@josephsavona
Copy link
Contributor

👍

Explicitly test the case where the slice is oversized/undersized on the
left/right.
wincent added a commit that referenced this pull request Oct 12, 2015
Make connection adapter for array slices
@wincent wincent merged commit 03076f3 into graphql:master Oct 12, 2015
wincent added a commit to wincent/masochist that referenced this pull request Nov 3, 2015
This is currently a pending PR upstream
(graphql/graphql-relay-js#31).

I naughtily just copied it into `node_modules`, so if, for whatever
reason, it can't be merged, I'll need to switch to a fork or find an
alternative.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants