Add usePageSwitcherPagination hook by philipp-spiess · Pull Request #44664 · sourcegraph/sourcegraph-public-snapshot

philipp-spiess · 2022-11-21T14:41:54Z

This PR is part of the strategic readiness effort to improve pagination.

We're adding new primitives to help us build more-robust and proper cursor-based bi-directional pagination. This PR adds a new hook, usePageSwitcherPagination which is a fork of useConnection with a few changes:

It requires a bi-directional connection (one that can be traversed forwards and backwards).
It support syncing the page state to the URL (like useConnection does).
It does not support custom filter or sort orders yet. However, the new hook is designed in a way where we will be able to do this easily and more reliably than useConnection.
It does not support periodic refreshment (polling). If this is needed, I suggest we export a refresh/reload function and implement this logic outside of this hook.
It has slightly different type-requirements. Specifically the new usePageSwitcherPagination hook requires you to give up all pagination related props to the hook. You can only manage initial page sizes but every subsequent page change is controller for you. This way we avoid the confusion when users have to pass first: and after: manually right now with the existing useConnection hook, even though it's thrown away after the first page load (and sometimes not even used at all if e.g. state from the URL is restored).

Everything is designed with the new pagination controls in mind which I'll be adding in a new PR:

Notes

This PR will have a failing CI until we add the backend support and at least one GraphQL resource that eposes the backward-pagination properties. This means that merging this PR is currently blocked.

Test plan

Added unit tests to test all of the new features. I've also tested it agains the WIP PR #44347

App preview:

Web

Check out the client app preview documentation to learn more.

taras-yemets · 2022-11-24T18:38:31Z

Can search contain before and after values with are not valid paginated connection query arguments (e.g., '?before=john&after=doe')? If yes, should we validate these values before using them in query?

The tokens are opaque so we can't do any form of validation (the backend will do that when we query it, though).

And no, before and after at the same time is not allowed. That's why i made it so there's only a after or a before branch. We never set both arguments at the same time (after takes presedenc but it doesn't really matter)

@philipp-spiess @taras-yemets this might be super obvious but I just recently noticed that this is a valid URL if you have more than one value of the same query param, for example

var s = new URLSearchParams('?site=x&site=y') s.getAll('site') // ["x", "y"]

this is of course a corner case but I think Taras may be right here, what if we have another consumer on the page that uses the same query params. Should we add console.warn if we find more than one pagination-like query param?

Super minor, feel free to ignore.

Should we add console.warn if we find more than one pagination-like query param?

I’m not sure how we could detect this scenario though. When we load the hook and detect pagination-like query params, we currently assume it is from the current pagination (there's nothing to identify it with, unfortunately).

FWIW this behavior has been a part of the existing pagination related abstractions as well (the legacy class component as well as the old useConnection hook).

philipp-spiess · 2022-11-28T11:10:30Z

This PR is currently blocked by the backend changes that @thenamankumar is working on because it uses a real GraphQL query in the tests that doesn't have the after and last fields implemented yet 😅

0xnmn · 2022-12-05T14:03:36Z

Do we want to have a default page size at backend? Given the args are naturally not null.

Right now its 100 which is same as the max page size allowed. Shall we have a separate default page size? 20?

I think at the backend if no first or after is set, we should error. WDYT?

Checked github api. That is what they do too. So yeah I'll add err in the backend.

pushed change to: https://github.com/sourcegraph/sourcegraph/pull/45167/commits

0xnmn · 2022-12-05T14:08:19Z

Love this. Having the url sync part of the connect hook, makes everything so simple and smooth.

0xnmn · 2022-12-05T14:09:18Z

Not approving because the BE PR is not merged yet. Otherwise LGTM.

valerybugakov

Great stuff, @philipp-spiess!

valerybugakov · 2022-12-06T08:17:41Z

From the Apollo Client documentation, it seems that refetch() calls use each omitted variable's original value. Would it be possible to remove the logic related to initialQueryVariables to simplify the hook?

Ooh that's nice, ty!

valerybugakov · 2022-12-06T08:19:09Z

Beautiful tests! 🔥

valerybugakov · 2022-12-06T11:17:57Z

Do we still need to useMemo initialQueryVariables if we don't use it in the updatePagination function? I thought that it would be nice to remove it if this is possible.

@valerybugakov I wasn't sure, I need to do more research. I was worried that a new variables object passed on every re-render with the previous pagination values might cause some headache and I only want it to reset when the user-supplied variables change.

@philipp-spiess I think the last time I checked useQuery hook used this package for deep variable and query options comparing https://github.com/benjamn/wryware/blob/main/packages/equality/src/equality.ts

Feel free to leave this useMemo call here this is more like FYI.

@vovakulikov Thanks! I removed it for now as I'd rather add it when I know it's necessary.

…n hook

…lways add back the gql call here)

philipp-spiess · 2022-12-06T14:35:00Z

I removed the dependence on a GraphQL schema that implements both forward and backward navigation (stubbed out the types) so we can ship this independently. Will update the types to run against the real schema once we have migrated at least one connection.

sg-e2e-regression-test-bob · 2022-12-06T14:41:14Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (0.00 kb)	0.00% (0.00 kb)	0.00% (0.00 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits a68593b and daeedc4 or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

cla-bot Bot added the cla-signed label Nov 21, 2022

github-actions Bot added the team/code-exploration Issues owned by the Code Exploration team label Nov 21, 2022

philipp-spiess self-assigned this Nov 21, 2022

philipp-spiess marked this pull request as ready for review November 21, 2022 15:52

philipp-spiess marked this pull request as draft November 21, 2022 15:53

philipp-spiess changed the title ~~Add usePaginatedConnection hook~~ Add usePageSwitcherPagination hook Nov 22, 2022

philipp-spiess mentioned this pull request Nov 22, 2022

[Meta] FE scale audit: Reusable patterns for proper cursor-based pagination #44706

Closed

7 tasks

philipp-spiess requested review from a team, 0xnmn and courier-new November 23, 2022 15:35

philipp-spiess marked this pull request as ready for review November 23, 2022 15:35

taras-yemets reviewed Nov 24, 2022

View reviewed changes

Comment thread client/web/src/components/FilteredConnection/hooks/usePageSwitcherPagination.ts Outdated

taras-yemets reviewed Nov 24, 2022

View reviewed changes

Comment thread client/web/src/components/FilteredConnection/hooks/usePageSwitcherPagination.ts Outdated

taras-yemets reviewed Nov 24, 2022

View reviewed changes

Comment thread client/web/src/components/FilteredConnection/hooks/usePageSwitcherPagination.ts Outdated

taras-yemets reviewed Nov 24, 2022

View reviewed changes

philipp-spiess mentioned this pull request Nov 25, 2022

Rename useConnection to useShowMorePagination #44703

Merged

philipp-spiess added scale-audit/pagination scale-audit/eng blocked labels Nov 28, 2022

philipp-spiess force-pushed the ps/use-paginated-connection branch from b2a82ac to b77450c Compare December 2, 2022 15:21

0xnmn reviewed Dec 5, 2022

View reviewed changes

limitedmage mentioned this pull request Dec 5, 2022

Search contexts usability improvements implementation [tracking issue] #44903

Closed

20 tasks

valerybugakov approved these changes Dec 6, 2022

View reviewed changes

valerybugakov reviewed Dec 6, 2022

View reviewed changes

philipp-spiess added 2 commits December 6, 2022 15:27

WIP add usePageinatedConnection

acbdfe6

WIP add test and make first page navigatnion work

940cd81

philipp-spiess and others added 12 commits December 6, 2022 15:27

Add remaining functions

731666d

More test cases, backward pagination working

2c3a1a7

minor fe integration efforts & bug fix

d4d18d2

Add URL sync logic, fix tests, and clean up new usePaginatedConnectio…

30b884d

…n hook

Revert search/backend.tsx change

2a33ef9

Clean up imports

249a5f6

Rename usePaginatedConnection to usePageSwitcherPagination

0374d80

Cleanup

678aa3a

Remove eager loading indicator, this will be handled in <PageSwitcher />

416aa45

Simplify refetch logic by not having to pass initial values

09b3050

Remove no-longer needed initialQueryVariables

6e4eebb

Remove dependency on real GraphQL fields to unblock this PR (we can a…

63d39ab

…lways add back the gql call here)

philipp-spiess force-pushed the ps/use-paginated-connection branch from cad93b8 to 63d39ab Compare December 6, 2022 14:33

philipp-spiess removed the blocked label Dec 6, 2022

Remove unused import

a68593b

philipp-spiess merged commit 66a455a into main Dec 6, 2022

philipp-spiess deleted the ps/use-paginated-connection branch December 6, 2022 15:39

Conversation

philipp-spiess commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Test plan

App preview:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipp-spiess commented Nov 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnmn commented Dec 5, 2022

Uh oh!

valerybugakov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valerybugakov Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipp-spiess commented Dec 6, 2022

Uh oh!

sg-e2e-regression-test-bob commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

philipp-spiess commented Nov 21, 2022 •

edited

Loading

valerybugakov Dec 6, 2022 •

edited

Loading

sg-e2e-regression-test-bob commented Dec 6, 2022 •

edited

Loading