New audius-query hook for paginated queries #9592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

DejayJD merged 6 commits into main from jd/aquery-pagination-changes

Aug 30, 2024

Contributor

DejayJD commented Aug 30, 2024 •

edited

Loading

Description

Moves the useAllPaginatedQuery hook inside of createApi and creates a new query type 'paginatedQuery' (in addition to query and mutation)

Another big change is that this now uses useSelector to accumulate data instead of managing an array in hook state. This means that optimistic mutation updates (i.e. api.utils.updateQueryData) now will reflect properly in paginated query hooks.

I also combined the two usePaginated queries (now its just an option singlePageData), since they were basically the same but just returned one page of data vs all pages.

Finally, this PR just marks the old hooks as deprecated for now; I didn't want to take on all the scope of refactoring & testing every pagination case at the moment. Will need to make a ticket to update old usages.
Open to pushback here though if we feel now is a good time to refactor the deprecated spots

imo this change makes it easier to set up paginated query hooks and works well since most of the time a query will either be paginated or not, very likely not needing both. Simplifies the hook usage too.

How Has This Been Tested?

Tested with the comments API (on a diff branch)


          New method for paginated queries

6841f98

DejayJD requested a review from amendelsohn

August 30, 2024 16:16

changeset-bot bot commented Aug 30, 2024 •

edited

Loading

⚠️ No Changeset found

Latest commit: db58d64

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

pull-request-size bot added the size/L label

DejayJD changed the title ~~New method for paginated queries~~ New audius-query hook for paginated queries

Collaborator

audius-infra commented Aug 30, 2024

Preview this change https://demo.audius.co/jd/aquery-pagination-changes

DejayJD requested a review from sliptype

August 30, 2024 16:40


          Fix lint

efdcfd1

Collaborator

audius-infra commented Aug 30, 2024

Preview this change https://demo.audius.co/jd/aquery-pagination-changes

sliptype approved these changes

View reviewed changes

Contributor

sliptype left a comment

I like this a lot better!

packages/common/src/audius-query/types.ts Outdated Show resolved Hide resolved

amendelsohn approved these changes

View reviewed changes

Contributor

amendelsohn left a comment

This is awesome! Thank you for taking on this work.

Love the move to include pagination in createApi

packages/common/src/audius-query/createApi.ts Outdated

+                  const hardReset = useCallback(() => {
+                    softReset()
+                    setStatus(Status.LOADING)
+                    // @ts-ignore - Unclear whats wrong with they type here

Contributor

amendelsohn Aug 30, 2024

lmk if you want to debug this type together

Contributor Author

DejayJD Aug 30, 2024

I started to dig into it but I saw some other spots in this file also ts-ignore-ing lol so I assumed maybe a limitation we haven't figured out?

packages/common/src/audius-query/createApi.ts

+                  // Since this is a paginated query we know that the results will be in an array
+                  // Call the useQuery hook to trigger cache checks and/or fetches
+                  const result = useQuery(args, queryHookOptions) as QueryHookResults<Data[]>

Contributor

amendelsohn Aug 30, 2024

I think we might run into trouble here because the legacy cache hits don't get written to the api slice. So calling this doesn't populate the audius-query store and your selector above won't find the data.

This is only an issue with a paginated endpoint that also provides idArgListKey (e.g. getTracksByIds)

Contributor

amendelsohn Aug 30, 2024

we could change the selector to be able to hit the legacy cache, or maybe write to audius-query cache on cache hit inside useQueryState

Contributor Author

DejayJD Aug 30, 2024

I dont think I'm following you? This didn't really change much from useAllPaginatedQuery, previously this was just being passed in but now we can just use it since it's in scope already

packages/common/src/audius-query/createApi.ts Outdated Show resolved Hide resolved

packages/common/src/audius-query/createApi.ts Outdated

Comment on lines 650 to 651

		const stillLoadingCurrentPage =
		loadingMore \|\| result.status === Status.LOADING

Contributor

amendelsohn Aug 30, 2024

I think this loadingMore is what that comment was mentioning about having another RELOADING status.

I think local state is probably fine here, but we could also add a state to the enum

packages/common/src/audius-query/createApi.ts

Comment on lines +673 to +674

		// If not in singlePageData mode, we treat the status based on whether the first page succeeded. After that, use isLoadingMore for load statuses
		allPageData?.length > 0 && !singlePageData ? Status.SUCCESS : status,

Contributor

amendelsohn Aug 30, 2024

this would be the case for a RELOADING or LOADING_MORE status

not sure if it's worth having just for this, or if it's easier or harder for the client to manage

Contributor Author

DejayJD Aug 30, 2024

I definitely like LOADING_MORE; I think that could easily replace isLoadingMore and solve for this quirk here
RELOADING I'm not as sure about; what would be the purpose?

Contributor Author

DejayJD Aug 30, 2024

This is a good time to discuss it though since we're not refactoring old stuff just yet

packages/common/src/audius-query/createApi.ts Outdated Show resolved Hide resolved

packages/common/src/audius-query/types.ts

+                 */
+                pageSize: number
+                /**
+                 * Toggles single page data mode. If true the hook will only return the current page's data. Equivalent to usePaginatedQuery in the past

Contributor

amendelsohn Aug 30, 2024

maybe document contrasting behavior when it's false? maybe it's obvious, idk

Contributor Author

DejayJD Aug 30, 2024

Hmm I feel like it should be somewhat obvious? I dont love the name though,

packages/common/src/audius-query/types.ts Outdated Show resolved Hide resolved

packages/common/src/audius-query/types.ts Outdated Show resolved Hide resolved

DejayJD added 4 commits

August 30, 2024 14:45


          PR feedback and update readme

d959e18


          type tweak

8295fea


          Merge branch 'main' of github.com:AudiusProject/audius-protocol into …

6729ca8

…jd/aquery-pagination-changes


          Remove RELOADING status

db58d64

Collaborator

audius-infra commented Aug 30, 2024

Preview this change https://demo.audius.co/jd/aquery-pagination-changes

DejayJD enabled auto-merge (squash)

August 30, 2024 22:20

DejayJD merged commit ed697c2 into main

DejayJD deleted the jd/aquery-pagination-changes branch

August 30, 2024 22:44

audius-infra pushed a commit that referenced this pull request


          Audius Client (Web and Mobile) v1.5.97

b24b804

[58bdb34] [PAY-3324] Purchase/Sales details w/ take rate (#9602) Marcus Pasell
[ebf9040] Support negative fractional numbers in FixedDecimal, default USDC formatting (#9600) Marcus Pasell
[fa2ac4c] [C-4953] Mobile comments editing + replying (#9584) Sebastian Klingler
[ed697c2] New audius-query hook for paginated queries (#9592) JD Francis
[847c075] [PAY-3319][PAY-3399] Update Discovery to support new purchase transaction details (#9586) Marcus Pasell
[760bfee] Center long track/collection titles on mobile (#9598) Reed
[c18d431] [C-4799] Add mobile-web comments (#9578) Dylan Jeffers
[74ab87e] [PAY-3388] Add user feed V1 endpoint (#9583) Randy Schott
[f9ae82a] [QA-1510] Simplify hover gradient styles coming from harmony (#9590) Raymond Jacobson
[86343d1] Fix search filters (#9595) Sebastian Klingler
[398190f] [QA-1498] Reland fix to QA-1498 (#9582) Raymond Jacobson
[0b92de2] Revert "Enabling blocking users from commenting (#9469)" (#9552) Isaac Solo
[27b6332] [QA-1531][QA-1535] Fix mobile track page issues (#9589) Raymond Jacobson
[7649131] Remove quality toggle for mobile downloads (#9587) Reed
[92737e3] [PAY-3372] Add sales aggregate endpoint (#9585) Andrew Mendelsohn
[ef67166] [QA-1536] Remove download all buttons (#9581) Reed
[83bb4bb] Set default comment entity_type in indexing (#9580) Dylan Jeffers
[331111e] Update dapp-store build artifacts audius-infra
[d08cce0] Fix mobile composer input newlines and unfurl overflow (#9575) Raymond Jacobson
[7eb83d7] Fix broken offline mode (#9571) Raymond Jacobson
[906f90a] [PAY-3377] Migrate remaining apiClient endpoints to SDK (#9559) Randy Schott
[c908aef] Fix mobile comment overflow menu lint (#9577) Dylan Jeffers
[0188a6a] [QA-875][QA-850] Fix lineup pagination when dupes are present (#9572) Raymond Jacobson
[c83ca67] [PAY-3394] Maintain chat list sort order on blast (#9568) Reed
[7470a55] [PAY-3397] Fix mobile layout with UserGeneratedText in DMs (#9570) Raymond Jacobson
[2c39178] [PAY-3374] Fix selection colors for composer (#9565) Raymond Jacobson
[78a028e] Update comment-section-api (#9551) Dylan Jeffers
[fedd220] [PAY-3384] Implement web pay with anything (#9279) Raymond Jacobson
[51e911f] [C-4981] Comments desktop tweaks (designer QA) (#9548) JD Francis
[fa43c8d] [PAY-3152] Fix Accounts You Manage modal showing after switching accounts (#9558) Randy Schott
[39c7764] [PAY-3371] Add endpoint to get tracks by user that have been remixed (#9554) Andrew Mendelsohn
[39a1ec1] [QA-1526] Hotfix pagination issues (#9557) JD Francis
[cb1446c] Fix types on harmony virtualized (#9553) Raymond Jacobson
[c4965de] [C-4955] Comment action drawer (#9534) Sebastian Klingler
[713712e] [PAY-2919] Migrate playlists endpoints from apiClient -> sdk (#9535) Randy Schott
[f8c197f] [QA-1016] Fix playlist creation from track added time (#9529) Raymond Jacobson
[ba1aaed] Fix trending reward notifs (#9532) Isaac Solo
[303614d] Add virtualized filter button, Fix option select (#9543) Dylan Jeffers
[97ac58e] Bump mobile apps to 1.1.112 Dylan Jeffers
[c10b064] [PAY-3348] Remove dead withdrawals code (#9542) Marcus Pasell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels