Bundle size report 📦

Look at the Statoscope report for a full comparison between the commits 03d4fa4 and 5473007 or learn more.

Codenotify: Notifying subscribers in CODENOTIFY files for diff 5473007...03d4fa4.

Another option could be to rename useConnection to useFetchMore (fetchMore is one of the functions returned by the hook) and use usePagination name for a hook returning prev/next page fetchers.

I like that but I’m not sure if pagination alone clear enough (we've discussed this with @felixfbecker in the Slack thread).

I'm still not sure about the naming.
We are going to have two useSomeTypeOfPagination hooks, and we are going to use these and derived hooks to get some kind of connection (e.g., useImportingChangesets, useBulkOperationsListConnection, RepoRevisionSidebarSymbols, etc.).
If I use some hook to get a connection and some functions to manage it, I'd probably expect this hook to have word 'connection' in its name. If I use useSomeTypeOfPagination I'd expect to get a current page number, total pages, and get prev/next page functions (for example these usePagination examples: 1, 2).
Having const { connection, error, loading, hasNextPage, fetchMore } = useShowMorePagination(...args) with connection being the 'main' return value does not seem intuitive to me.

@taras-yemets Why is the word connection so important in this case? E.g. cant useBulkOperationsListConnection also be called useBulkOperationsListPagination?

If I use useSomeTypeOfPagination I'd expect to get a current page number, total pages, and get prev/next page functions

In both cases you'd get a 'next page' function (fetchMore and goToNextPage/goToPreviousPage) but yeah we can't show you the current page number because that's not possible with cursor based navigation. I still think that there's not a fundamental difference between this hook, the one I’m adding and the ones you linked.

Having const { connection, error, loading, hasNextPage, fetchMore } = useShowMorePagination(...args) with connection being the 'main' return value does not seem intuitive to me.

I think connection still makes sense as a term here because this is what GraphQL is calling the type of this thing. We can probably also return the current nodes instead because connection must have nodes, would that be better?

@philipp-spiess, I don't have name suggestions I'm confident in that seem more suitable than the ones shared by folks in this PR and in the referenced thread.
I'm fine with the selected name.

@philipp-spiess I'm not intimately familiar with the pagination effort, just saw this PR and was interested. I skimmed through the Slack convo and the related tracking issue, and other PRs, that's pretty much all the context I have. So just leaving my two cents here:

As far as I know, the term "connection" comes from the "GraphQL Cursor Connections" (I'll call it GCC in this message) standard: https://relay.dev/graphql/connections.htm
I guess the name useConnection wants to mean that "this is a hook that connects to a GraphQL API that follows the GCC standard, and returns a connection as defined in the GCC spec."

I understand @thenamankumar is working on back-end changes that improve our pagination. But I'm unsure: does it also mean that we're moving off the GCC standard?

a.) If yes, then renaming the hook to something like useShowMorePagination makes a lot of sense to me because it abstracts away GCC, and basically says that "this is a hook that can handle pagination with some backend (that is not necessarily GCC-compatible)".
b.) If no, then I think keeping the term connection in the hook name is pretty elegant because it compresses a lot of information (a reference to the GCC standard) in a very short term.

Again, just my two cents, I think this rename can't hurt in any case, as it's very easy to change it later.

Thanks @vdavid!

I understand @thenamankumar is working on back-end changes that improve our pagination. But I'm unsure: does it also mean that we're moving off the GCC standard?

No! The work that we're doing is to extend upon the GraphQL connection interface and add a helper in Go that makes it easier to: 1) uses proper cursors (the current patterns all show lead to OFFSET .. LIMIT .. based SQL queries which defeats a lot of the purpose here) and 2) allows for forward and backward navigation.

We still expect the GraphQL APIs to implement the Connection type.

If no, then I think keeping the term connection in the hook name is pretty elegant because it compresses a lot of information (a reference to the GCC standard) in a very short term.

That is fair but I wonder how much additional value the knowledge that a hook uses an underlying GraphQL connection really brings: When adding a new usage of the hook, the types already make it clear that this is the required backend structure and when reading the code I feel like this term has just become a synonym for pagination in our codebase.

Given that we expect this to exist (for at least some period of time) in combination with the new usePageSwitcherPagination hook, it makes sense to me for this one to be named useShowMorePagination. For now, they both are clearly distinguishable about what they should be used for until we are able to support both cases with a single implementation.

We can provide more context/detail about the relevant implementation details (e.g. the use of a "connection") as needed/desired in the JSDoc comments for the hooks (and, like you mentioned @philipp-spiess, the type system also helps us describe this).

Thank you all for your feedback! I’m still not sure about the Connection vs Pagination suffix but since nobody felt really strongly I’m going to bias toward action and ship my current proposal. We can always rename things later if we need that 😅