Skip to content
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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mAAdhaTTah
Copy link

Implemented in accordance with the RFC. 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.

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
@mAAdhaTTah mAAdhaTTah marked this pull request as draft June 23, 2022 13:50
@codesandbox-ci
Copy link

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:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration
SWR-SSR Configuration

@mAAdhaTTah
Copy link
Author

Just saw #2018 as I was opening this, whose implementation seems a lot simpler. I think dealing with an array of data (as per Proposal A) results in a simpler implementation. The "Array of SWRs" approach makes it difficult/impossible to use useSWR under the hood the way #2018 does, because we're not just dealing with a single piece of data, but all of the associated pieces (mutate, loading state, etc.).

There's definitely a ton of duplication, but I didn't want to start seriously refactoring useSWR itself to extract more reusable bits without feedback because that will def result in a lot of churn. I also am not 100% clear around which of these functions need to be stable cuz there's a lot of disabling of exhaustive deps in useSWR that I'm not fully familiar with.

cc @shuding @promer94

@mAAdhaTTah
Copy link
Author

Also looking at #2006, which interestingly did some things similarly to the way I did so that's cool.

@shuding
Copy link
Member

shuding commented Jun 25, 2022

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 data. (cc @promer94)

@mAAdhaTTah
Copy link
Author

We should evaluate the difficulty of making the core support a list of keys.

Funny enough, as I was doing this, my galaxy brain thought was that, instead of trying to implement useSWRList as "useSWR with an array of keys", we should do the reverse and implement useSWR as "useSWRList but only a single key".

In terms of landing this, since v2 is still in beta, I think we have a few options:

  1. Land this "as-is" (except with more tests), do some additional refactoring as follow-on PRs.
  2. Extract more helpers from core first, then refactor this PR to take advantage of them.
  3. Use this PR to do a bigger refactor of and land it all at once.
  4. Something else?

Thoughts? How should I proceed?


A few things I did like from @promer94's implementations:

  1. Returning results as a key on an object gives the API more flexibility to expand in the future.
  2. Or even expand now, with a root mutate, isValidating, isLoading that applies to all of the keys.
  3. Adding the key (and possibly originKey) to the returned object also seems useful.

I'm not so much a fan of the children/ items of useSWRAggregator, which seemed unnecessary, and in general, I think the API of useSWRList, providing the array of SWR objects, is generally more useful than just an array of data.

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.

@promer94
Copy link
Collaborator

Concerns

Returning 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.
This is why react-query’s useQueries , IMO, is really a bad api design.
SWR always proivder greate performance for users. So i want to keep that.

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
}

@promer94
Copy link
Collaborator

children/ items of useSWRAggregator is useful because it will have much batter performance and user can even access the result array in individual item component and it saves a lot of boilerplates

@mAAdhaTTah
Copy link
Author

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. fallback is a little odd because it would probably have to be structured like fallbackData rather than fallback in the original. Alternatively, we could make fallback an array as well.


One of the weird things about the children / items approach is that, because an individual item is subscribing directly to its key, you could feasibly end up in a place where an individual item is rendered because its data is complete but the aggregated result is still loading and thus empty.

Relatedly, there isn't any way to invalidate/mutate an individual key at the top level with useSWRAggregator. If you have a large list & need to invalidate a subset of those keys in a bulk update, you probs have to pull in the top-level mutate from useSWRConfig to accomplish it. That said, I don't think this would be difficult to add to that PR and shouldn't change the performance characteristics.

const results = serializedKeys.map(([key], index) => {
if (!(index in keysRef.current)) keysRef.current[index] = key

if (!(index in stateDependenciesRef.current))
Copy link
Collaborator

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

Copy link
Author

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.

Copy link
Collaborator

@promer94 promer94 Jun 27, 2022

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.

@mAAdhaTTah
Copy link
Author

So where are we with this PR? Do we need to revisit the RFC and change the API design? @shuding @promer94

@mAAdhaTTah
Copy link
Author

Hi all! Revisiting this again. Would love to push this or some sort of multi-key implementation forward. How can we advance this?

@0xbe1
Copy link

0xbe1 commented Dec 2, 2022

any plan on crossing the finish line? look forward to using this feature 🙏

@mAAdhaTTah
Copy link
Author

@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.

@mAAdhaTTah
Copy link
Author

@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.

@mAAdhaTTah
Copy link
Author

@promer94 @shuding Another ping to say I'm still interested in pushing this forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants