swr icon indicating copy to clipboard operation
swr copied to clipboard

Initial implementation of `useSWRList`

Open mAAdhaTTah opened this issue 2 years ago • 14 comments

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.

mAAdhaTTah avatar Jun 23 '22 13:06 mAAdhaTTah

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 8ca9b806907092f8da30996840ccad9a23b89d2d:

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

codesandbox-ci[bot] avatar Jun 23 '22 13:06 codesandbox-ci[bot]

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 avatar Jun 23 '22 13:06 mAAdhaTTah

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

mAAdhaTTah avatar Jun 23 '22 13:06 mAAdhaTTah

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)

shuding avatar Jun 25 '22 12:06 shuding

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.

mAAdhaTTah avatar Jun 25 '22 13:06 mAAdhaTTah

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 avatar Jun 25 '22 15:06 promer94

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

promer94 avatar Jun 25 '22 15:06 promer94

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.

mAAdhaTTah avatar Jun 26 '22 14:06 mAAdhaTTah

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

mAAdhaTTah avatar Jul 15 '22 14:07 mAAdhaTTah

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

mAAdhaTTah avatar Nov 07 '22 18:11 mAAdhaTTah

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

0xbe1 avatar Dec 02 '22 08:12 0xbe1

@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 avatar Dec 02 '22 13:12 mAAdhaTTah

@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 avatar Mar 28 '23 20:03 mAAdhaTTah

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

mAAdhaTTah avatar Jun 30 '23 20:06 mAAdhaTTah