(graphcache) - Add mergeMode to simplePagination helper

[hoangvvo]

Resolves #1173

Summary

Set of changes

Adds mergeMode to simplePagination similar to that of relayPagination. Default to inwards, so it will not be a breaking change.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6f1231a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@urql/exchange-graphcache	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[JoviDeCroock]

Looks good! Thank you for taking the time to PR this, mind adding a changeset by running yarn changeset at the root? This would be a minor for @urql/exchange-graphcache since we're adding a pagination feature! 👍

[hoangvvo]

Thanks for such a quick review! I added documentation for mergeMode of Simple Pagination based on that of Relay Pagination. Changeset also added.

[kitten]

I believe this is misunderstanding what the mergeMode does on Relay pagination.
There it takes care of which order things are merged in, yes, but in relation to how the pagination arguments are used, since before and after cursors can be used for the same list. This then creates ambiguity because both directions can be "outward" from the same point or "inward" towards a shared centre in the list.

I believe mergeMode may not make much sense for simplePagination as we're not dealing with two directions of pagination so inwards/outwards aren't descriptive.

Furthermore I'm unsure the explanation in the RFC is accurate and whether a lot of APIs even use this pattern? Do we have some prior art here?

[hoangvvo]

@kitten You are right. I am not familiar with Relay and possibly misusing the term inwards and outwards. Either way, here are the relevant words from relay pagination definition:

Outwards pagination assumes that pages
that come in last should be merged before the first pages, [...] The default inwards pagination assumes that
pagination last pages is part of the same list and come after first pages.

This matches the description of what I am trying to achieve. Perhaps, mergeMode = "before" | "after" or mergeMode = "reverse" | "forward" might be better. It satisfies use-cases of infinite scroll but instead of scrolling down for older items, we scroll up for them. The GIF above demonstrates that.

[kitten]

@hoangvvo i believe would rename it like that 👍 the reason why the description matches is because for this paragraph it describes rather what happens when we start pagination from in reverse while forward pagination has been used and vice-versa. So the behaviour does match but it's for a subtly different reason 😅

[hoangvvo]

@hoangvvo i believe would rename it like that the reason why the description matches is because for this paragraph it describes rather what happens when we start pagination from in reverse while forward pagination has been used and vice-versa. So the behaviour does match but it's for a subtly different reason

I agree that it is still a somewhat uncommon pattern. I can definitely workaround this by simply reverse the returned GraphQL array appropriately.

offset = 0 => 9,8,7
offset = 3 => 6,5,4

The current simplePagination will merge it into [9,8,7,6,5,4] and I would only need to reverse it to achieve the above effect. So it is okay to not putting this into the helper.

Otherwise, how would you rename it? I am steering toward mergeMode = "before" | "after" with the default being after, where pages with higher offset will be put after, and can otherwise be "before" where pages with higher offset will be put before. The doc will also be updated to reflect this instead of using the terms inwards and outwards:

offset = 0 => 7,8,9
offset = 3 => 4,5,6

mergeMode = "before" => [4,5,6,  7,8,9]
mergeMode = "after" => [7,8,9,  4,5,6]
```

[hoangvvo]

@kitten I updated the RFC to clarify the intention and approach

[kitten]

Awesome! Yea, I think 'before' | 'after' is definitely the most intuitive solution 👍

[kitten]

There's one more section in the docs that needs updating, but I can totally do that after this PR myself if you'd like ✌️ https://github.com/FormidableLabs/urql/blob/main/docs/api/graphcache.md#simplepagination

Thanks for adding this!

[kitten]

Suggested change

	Add mergeMode = 'before' | 'after' to simplePagination helper to define whether pages are merged before or after when you paginate forwards and backwards
	Add a `mergeMode: 'before' | 'after'` option to the `simplePagination` helper to define whether pages are merged before or after preceding ones when pagination, similar to `relayPagination`'s option

Just suggesting some details here since we can use full markdown in our changelog 😇

[kitten]

Suggested change

	You can also specify `mergeMode`, which defaults to `after` and can otherwise
	be set to `before`. This will handle how pages are merged when you paginate
	We may also add the `mergeMode` option, which defaults to `'after'` and can otherwise
	be set to `'before'`. This will handle in which order pages are merged when paginating

We usually use "we" as often as possible when writing (with some exceptions). I realise the Graphcache pages may need some revamping to make sure they're as helpful as can be compared to some others, but let's take that into account here

[hoangvvo]

Thanks for the review, I updated the documentation and also added to graphcache.md.