-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
[API Concept] - Infinite Query API #4393
[API Concept] - Infinite Query API #4393
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4ee0f7f:
|
Hi! I finally found some free time today to start doing some research on infinite query APIs in other libs like React Query, and am starting to wrap my brain around this space a bit. I haven't tried to dig through this PR yet, but I definitely endorse the idea of I am trying to understand the space enough to have actual feedback, but as a very first starting point, one goal would be that this ought to work as part of the UI-agnostic RTKQ core, so that any UI layer can use it (React, Angular, etc). Looking at React Query's impl, there's a core I haven't even looked at your code yet, so I'm not sure what the specific implementation approach is, but that's the kind of ideal approach I'd like to end up with. Let me see if I can rebase this PR so it's at least up to date and builds. |
21d642f
to
bfd8de6
Compare
Okay, I just rebased this. I probably broke things somewhere in the process - the rebase was complicated due to us having drastically rearranged a lot of the internal typedefs a few weeks ago. The one test added to the PR appears to run and pass. There's a dozen TS errors, which look to be related to places where we expect values like FWIW, @phryneas separately commented that he happens to like SWR's infinite query API a bit better:
I don't know enough about this space yet to have an opinion here :) I think it's worth pushing this branch forward and seeing if we can reach feature and test parity with React Query's behavior, but maybe also putting together another branch that takes more of an SWR-style approach and see how they compare. Also, I'm going to be seeing Dominik ( @TkDodo ) at React Advanced and we'll try to chat about this topic as well. |
Okay, I think this at least passes enough to let the CodeSandbox CI build succeed, so we should have a viable PR package preview to play with now! Off the top of my head, the big open questions are:
|
Hm, in my email notification there is a quote from @phryneas but it seems edited out now
Of course I defer to his experience and expertise, but it's kind of funny because it's something that seemed desirable for us... We have a direct message UI that uses a paginated API request, which we further enhanced with WebSockets for update/delete of existing messages and the addition of new ones. Currently we're using the If there are separate cache entries for each page, now we have to iterate through them all in order to find the entity to update/delete, and we're left with a dilemma of where to add new messages that came from the WebSockets instead of the paginated REST requests...now the page size will be out of sync if the user scrolls around and triggers new queries. (I guess with our current I'm super curious to understand what formed the opinion that "merging multiple pages into one cache object is not a good choice" :) and it would be great to have this use-case considered, as I imagine it could very well be a common one. |
Both ways are not fool-proof, but at this point I believe that having to maintain one big cache object can be a burden to the developer, and I would like to avoid going that route in a new implementation. Some thoughts: In cursor-based pagination, additions and deletions can work easier with multiple pages that get stitched.
With offset-based or page-based pagination, you kinda end up with the opposite problem, where the "big blob" approach seems simpler, but the SWR api helps here by passing in the So... cursor-based is in my eyes easier, offset-based not necessarily more easy or difficult, just different.
That could be done without iterating, with
That's why I like the SWR approach, where a page could grow or shrink without causing a lot of trouble. |
@phryneas let’s discuss this in person in London if you’re there, just some quick, high level thoughts:
I don’t think we have problems with pages shrinking / growing when you have one cache entry, as each page is still stored separately. It’s fine to have one request return 10 pages, and then the next one just returns 9. You can also just delete one item from a page, that doesn’t mean an item from the next page must be moved manually - it can just stay the way it is. One thing that’s a conscious decision for us to have one big cache entry is that it commits or errors (or retries) as one entity. That means the page only renders and receives an update after all refetches are done. Assume someone added an entry on the first page, which will “shift” all follow-up pages. If they are separate cache entries - wouldn’t you see the UI temporarily show duplicate entries? And wouldn’t it stay that way if fetching the 2nd or 3rd page fails?
This seems great - the cursor is an input to the other cache entries, making them refresh automatically. But it also means that updates to pages before my page aren’t reflected. If someone renames an entry on page 1 of 5, and I rename something on page 3 of 5, only 3,4,5 will refetch and I won’t know about the change on page 1. Also, refetches staring from a page in the middle can be weird with paginated queries. Suppose I change an entry on page 3 (pageSize=3) and someone deletes 3 entries before that:
Let’s rename H to X, while at the same time, C, D and E were deleted by others. If we only start to refetch with
so the entry we updated isn’t even visible anymore, while the database actually has:
so I think the only safe thing to do is to refetch all pages when you refetch an infinite query, from the start. At least this is what we’re doing and I’m trying to talk sense into that approach 😂 |
@TkDodo that sounds like you are much closer to the "multiple cache entries" than the "single cache entry" I'm arguing against - in our case, I'd use a selector to combine them into a single "outside visible cache entry", so I don't think we're far away from each other at all :) But yeah, let's definitely talk about this in London! |
This draft is in a rougher state (especially around types) as it was mostly me forcing some things to get it working, but given the activity on it again; I'll hop in and clean it up, set some tests and update the discussion on the state of it. Honestly need to refresh myself with the problem space as I haven't done anything here since opening it :D |
c55de7f
to
5ce06d1
Compare
Okay, spent the last couple days doing some significant work on this PR to wrap my head around it, understand what it does so far, and try out both the TS types and actual implementation to see what's lacking. I've pushed several updates:
This desperately needs more tests and fleshing out, but I feel like it's getting close to a point where it works enough that we can try it out and say "is this the right API design in general?", knowing that it at least is working the way we think this API design ought to work.
I do see this PR has been failing against TS 4.7 and 4.8 for a while. That'll need to be fixed eventually, but doesn't stop us from iterating on it and figuring out if this API design is the approach we want. |
dfb1093
to
2c39bf3
Compare
7e9087a
to
ffafe62
Compare
Pushed a bunch of fixes for
Also changed the pagination command |
Jotting down some todos as I think of them: Functionality
Tests / Example Use CasesReact Query examplesref: https://tanstack.com/query/latest/docs/framework/react/guides/infinite-queries
Other
|
For notification purposes: just edited the original PR description comment with an info-dump on the status of this PR and a bunch of todos. Basically: "TRY THIS OUT AND GIVE US FEEDBACK, I'M WORKING ON THIS!" |
I've been testing this out, and I've noticed that Steps to reproduce
|
Would the team be open to supporting pagination through the link header? I think this is a pretty nice API because it means we don't have to maintain any frontend code - the backend just says how to get the next page and we're done. If the backend decides to change how pagination works (for example, switching from page=1 to offset=100 or cursor=abcd etc.) then the UI can just pick that up and roll with it. Plus it's already a standard so there may be many APIs out there already implementing this. |
I built a simple example showcasing the use of an infinite query with the Pokémon API for others to explore |
@jack-bliss how does that work with React Query's API today, if at all? |
After learning more about how react-query works, I don't think it does at all. Mildly disappointing but we live and learn. |
I wouldn’t say it’s impossible - it’s just that, as a tool that isn’t tied to HTTP, we don’t offer any integration per default. But nothing stops you from:
pseudo implementation:
|
Also updated the infinite logic to look up the existing `{pages, pageParams}` from state, rather than passing it in to the thunk
# Conflicts: # packages/toolkit/src/query/core/buildThunks.ts # packages/toolkit/src/query/index.ts # packages/toolkit/src/query/react/buildHooks.ts
ffafe62
to
0a32a5c
Compare
Awright, here's the plan. This PR has been open for a while, and it's also currently in a separate repo. While I can push to that branch, other external folks who want to contribute can't easily do so. I've created a new That should allow anyone who wants to help contribute to do the normal fork+PR process against that integration branch, and the PRs will show up here as usual. |
1e5789f
into
reduxjs:feature/infinite-query-integration
Okay, the new integration PR is up! Please see that PR for all further development work and updates. |
Status Update, 2024-11-29 (by Mark)
The development work for the infinite query feature has moved to a new running integration PR:
Please see that PR for all updates and try it out!
Prior updates: