-
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
Initial implementation of useSWRList
#2047
base: main
Are you sure you want to change the base?
Conversation
Implemented in accordance with the [RFC][1]. Initial implementation is mostly a copy/paste of `useSWR`, then moved things into arrays so we can handle the list aspect of this correctly. Plenty of work still to be done, especially around the duplication between `useSWRList` & `useSWR`, plus more tests need to be written, but this gives us a basis for discussion. [1]: vercel#1988
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 8ca9b80:
|
Just saw #2018 as I was opening this, whose implementation seems a lot simpler. I think dealing with an array of There's definitely a ton of duplication, but I didn't want to start seriously refactoring |
Also looking at #2006, which interestingly did some things similarly to the way I did so that's cool. |
Totally agreed. It is also my biggest concern that we are not reusing the implementation of SWR's core. We should evaluate the difficulty of making the core support a list of keys. Another reason is that we still need to return an array of SWR objects, instead of an array of |
Funny enough, as I was doing this, my galaxy brain thought was that, instead of trying to implement In terms of landing this, since v2 is still in beta, I think we have a few options:
Thoughts? How should I proceed? A few things I did like from @promer94's implementations:
I'm not so much a fan of the I think for all of the above approaches, I'm thinking implementing those 3 ideas would be good, so unless you have any objections, I'll include those changes once we have a direction. |
ConcernsReturning an array of SWR objects could lead to bad performance.If we have 1000 keys, the component which uses useSWRList will rerender like 1001 times. Some Configuaration's semantics are not clear{
// should onSuccess be called multiple times with individual result and key ?
// or just once with list result and array keys ?
onSuccess: (data, key) => {
}
// Similar problem for
onError,
onErrorRetry
onDiscard
} {
// should we support fallback here ?
fallback
} |
|
FWIW, I agree with the performance concerns. I included that as a con of this approach in the original RFC. However, I think the semantics would derive directly from the idea of it just being an "array of SWRs": all of those would be called per-item. Obviously, this exacerbates the performance issues, but I don't think it's confusing. One of the weird things about the Relatedly, there isn't any way to invalidate/ |
const results = serializedKeys.map(([key], index) => { | ||
if (!(index in keysRef.current)) keysRef.current[index] = key | ||
|
||
if (!(index in stateDependenciesRef.current)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most tricky problem of "return an array of SWR objects" is how to update the stateDependenciesRef properly.
Updating them during the render process is not recommended by react team and is not concurrent safe.
This is actually the main reason why i close #2006
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SWR core updates the stateDependiesRef
on access, which typically occurs on render. Is this a problem for core? I think we do have a specific problem here because the user could add/remove keys, which does need to remain in sync, but maybe instead of doing that by index, we could do that by key? Then at least the behavior is closer to core than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i do have same concerns for the core. But i have tested core with this repo and it worked as expected. So i think the current implmentation of core is ok for now. I am still expolring a batter way to do it.
Hi all! Revisiting this again. Would love to push this or some sort of multi-key implementation forward. How can we advance this? |
any plan on crossing the finish line? look forward to using this feature 🙏 |
@0xbe1 I'm still happy to work on this, but without a clear direction on the API design, there's not much I can do. |
@promer94 @shuding Anything I can do to push this forward? We really need an API around this on our project. We've been working with a janky userland implementation that I would love to get rid of and replace with a blessed implementation, but without a direction or clear sign this can get merged in some form, we're at a bit of a standstill. |
Implemented in accordance with the RFC. Initial implementation is
mostly a copy/paste of
useSWR
, then moved things into arrays so wecan handle the list aspect of this correctly. Plenty of work still to be done,
especially around the duplication between
useSWRList
&useSWR
,plus more tests need to be written, but this gives us a basis for discussion.