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

new useQueries API #2923

Closed
TkDodo opened this issue Nov 11, 2021 · 22 comments
Closed

new useQueries API #2923

TkDodo opened this issue Nov 11, 2021 · 22 comments
Labels
Milestone

Comments

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 11, 2021

just taking an Array of useQuery objects doesn't cover all the cases, because some options have to be the same for all queries, for example suspense or useErrorBoundary: We can't really have one query in the list suspend and some other query not suspend. Conceptually, useQueries is also more than a bunch of useQuery calls chained together.

Proposed solution

make useQueries take an object as input, where we can define top level options that are true for all queries and a list of queries, something like:

useQueries({
    queries: [{ queryKey1, queryFn1, options1 }, { queryKey2, queryFn2, options2 }],
    suspense: boolean,
    useErrorBoundary: boolean
})
@TkDodo TkDodo added the v4 label Nov 11, 2021
@TkDodo TkDodo added this to the v4 milestone Nov 11, 2021
@arnaudbzn
Copy link
Contributor

arnaudbzn commented Nov 11, 2021

if a query has suspense:false and the top level option is suspense:true, what would be the best behavior?

  • remove suspense from the query option TypeScript type for useQueries
  • throw a runtime error
  • ignore the query option

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 11, 2021

omit it from the options on type level and ignore it at runtime is the plan.

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 11, 2021

If you know any other options that should be hoisted, please let me know :)

@arnaudbzn
Copy link
Contributor

omit it from the options on type level and ignore it at runtime is the plan.

I'm not a huge fan of "silent" anomalies at runtime (from the developer point of view who set the option at the query level and could expect a certain behavior).

You may have this case with the current useQueries where one query do use suspense and another don't.

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 11, 2021

You may have this case with the current useQueries where one query do use suspense and another don't.

suspense doesn't work at all for useQueries at the moment, and having one suspend and the other not is exactly the issue that sparked this re-design. There is an old PR (#2109) that tries do to it, and the best thing we could come up with is suspending as soon as any of the queries suspends. That's pretty much the same as this proposal, except that this is more explicit.

from the developer point of view who set the option at the query level and could expect a certain behavior

hmm, imho, the options just doesn't exist anymore on that level in v4. TypeScript will enforce that, and we'll also highlight it in the migration guide. It is equivalent to passing forceFetchOnMount: true to a query and expecting it to do something. This option existed in v2, but not anymore in v3.

@arnaudbzn
Copy link
Contributor

TypeScript will enforce that, and we'll also highlight it in the migration guide. It is equivalent to passing forceFetchOnMount: true to a query and expecting it to do something. This option existed in v2, but not anymore in v3.

It's fine with me!

@lubieowoce
Copy link

lubieowoce commented Nov 11, 2021

Some thoughts about mixing suspense settings within a component

Note: This wall of text mostly assumes that mixing suspense settings within a single component is a valid usecase, which reasonable people might disagree on. For an example of when you might want this, see my next comment, but replace "crash" with "suspend". I'm also assuming enabled: true for simplicity.

I originally agreed with this proposal, but now I'm not so sure. Here's how that went.
My original thinking was that an old-style mixed-suspense call like this:

// old API
useQueries([
  { ...A, suspense: false },
  { ...B, suspense: false },
  { ...C, suspense: true },
  { ...D, suspense: true },
])

can always be expressed as two separate calls:

// new API
useQueries({
  queries: [ A, B ],
  suspense: false,
});
useQueries({
  queries: [ C, D ],
  suspense: true,
})

so there's no problem with hoisting the setting out. but...

The issue

The issue is that in the current implementation, mixing suspense settings like this will have some unexpected behavior -
non-suspense queries establish the subscription (and begin fetching) in a useEffect, which won't run if the component suspends. So the non-suspense queries would only start fetching after all the suspense queries are done and the component mounts! Which is pretty bad [1].

Actually, useQuery has the same issue, in that

useQuery({ A, suspense: false });
useQuery({ B, suspense: true });

will behave like await B; await A instead of await Promise.all([A, B]). I'm not quite sure what to do about it, other than move everything over to fetch-during-render[2] instead of the current fetch-in-useEffect, and that feels a bit weird.

But... if we allowed mixing suspense within a single useQueries call, we can handle this case! i.e. if any queries have suspense: true, then we start fetching everything (during render) and suspend on until C & D are ready. Because unlike separate useQuery calls (that don't know about each other), useQueries has all the info available and can do the smart thing. Obviously that's a maintenance/implementaion burden, but it doesn't sound impossible.

Conclusion

At the end of the day, we could just say

If you mix suspense settings in a single component, you're gonna have a bad time. Don't do it!

But allowing it in useQueries, i.e. keeping the current API is actually the one situation where we can do "the right thing" without re-writing everything that uses useEffect. So maybe it's worth considering after all? If this takes a while to implement, we could throw an "Error: mixing suspense settings isn't supported yet", then adding it later would be backwards compatible!


[1] As in, we don't want to do the equivalent of this:

await Promise.all([C, D]);
await Promise.all([A, B]);

[2] One way to do this would be to do something like the following: (obviously super simplified)

function useQuery(...) {
  // ...
  // look ma, no useEffect!
  if (shouldFetch) {
    queryClient.scheduleFetch(options) // express an intent to fetch, but don't start it yet
  }
  // ...
}

function QueryClientProvider({ client: queryClient, children }) {
  // ...
  useEffect(() => {
    // Actually perform all the fetches scheduled during render.
    // Assuming there's a Suspense boundary below us, this will run
    // even if a child component suspends!
    queryClient.executeScheduledFetches();
  });
  // ...
}

This way we're doing just a little bit of side-effects, so ...maybe... it's alright to do it during render?

@lubieowoce
Copy link

lubieowoce commented Nov 11, 2021

I guess my feelings about useErrorBoundary are similar -- i think mixing them is actually a valid usecase. Something like:

// assuming `suspense: true` for brevity -- i don't feel like typing out `isLoading` stuff :)
const [
  { data: criticalData },
  { data: niceToHaveData }
] = useQueries([
  { ...criticalQuery,   useErrorBoundary: true }, // if this doesn't work, just crash, nothing we can do
  { ...niceToHaveQuery, useErrorBoundary: false }, // we can manage without this one though
]);

return (
  <div>
    <CriticalUI data={criticalData} />
    <NotThatImportantUI data={niceToHaveData ?? "it's okay, we can show a fallback or something"} />
  </div>

EDIT: of course, with a "hoisted" useErrorBoundary, the behavior desired in the example could technically be achieved by any variation of

niceToHaveQuery.queryFn
  .then(data => ({ ok: true, data }))
  .catch(error => ({ ok: false, error }))

but then react-query doesn't know the query failed, so you lose isError, retries, etc, which isn't great.

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 12, 2021

@lubieowoce thank you for your input. I really like the useErrorBoundary example you gave with critical / nice to have queries. That makes a lot of sense if we can make it work like that.

As for suspense, I have very little experience with it. From my last talk with @tannerlinsley, I took away that what we want to achieve is to use the same suspense boundary for all queries that do suspend and fetch them in parallel. The loading should stop when all queries are done.

if we mix suspense true / false within one useQueries, how would that behave? As you said:

the non-suspense queries would only start fetching after all the suspense queries are done and the component mounts!

Yes, you get basically the same with separate useQuery calls / separate useQueries calls, but at least it's explicit.

Changing the api now to an object will give us more room for extensibility in the future to have settings that need to be set for all queries in the list. I thought that suspense would be a good first candidate for it. I agree that useErrorBoundary could be kept separately though.

But allowing it in useQueries, i.e. keeping the current API is actually the one situation where we can do "the right thing"

I don't fully understand that. If users really want to mix suspense / non-suspense, couldn't they just do two separate useQueries with the new api?

[1] As in, we don't want to do the equivalent of this:

Why not? I think this would be the expected behaviour for me. How else should it be?

FYI, with React 18, we will likely move over to useSyncExternalStorage instead of manually fetching in a useEffect, and maybe we'll even someday move to a suspense-cache from react, but all of this sounds like the far away future.

@lubieowoce
Copy link

lubieowoce commented Nov 12, 2021

Why not? I think this would be the expected behaviour for me. How else should it be?

If users really want to mix suspense / non-suspense, couldn't they just do two separate useQueries with the new api?

unfortunately they can't. this won't start fetching [A,B,C,D] in parallel (which is what i personally would expect):

useQueries([A, B], { suspense: false }); // #1
useQueries([C, D], { suspense: true }); // #2

because when a component suspends (because of #2), its effects don't run, so #1 won't do observer.subscribe(...) and thus won't start. it'll only start when #2 completes and the component "resumes", rerenders & mounts. so, assuming we're staying with useEffect for now, i don't see a way to start all 4 in parallel without mixed-suspense. as mentioned before, the usecase is the same as my useErrorBoundary example above.

(obviously if we swapped the calls and did the suspense one first, i wouldn't expect them to start in parallel, because control flow wouldn't reach the non-suspense one during the initial render. but it's way less obvious that the order in the example won't be parallel)

please let me know if I can explain anything better! i tried my best to be explicit about the assumptions/expectations i have for the behavior, but maybe I'm missing something :)

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 12, 2021

unfortunately they can't. this won't start fetching [A,B,C,D] in parallel (which is what i personally would expect):

I think it would be fine to suspend two and then start the other two, but yeah, it's not ideal. It's also a question of "who does this?", and "is this really a use-case"? Why would you suspend one query but not the other?

i don't see a way to start all 4 in parallel without mixed-suspense.

the current suspense implementation in useQuery throws the promise during render:

https://github.com/tannerlinsley/react-query/blob/16baba415058a8317cd7fd588c98ab0d56ca6dd2/src/react/useBaseQuery.ts#L116-L129

not sure how useQueries would do that in a mixed array that the non-suspense things start to query while the others are suspending ?

please let me know if I can explain anything better! i tried my best to be explicit about the assumptions/expectations i have for the behavior, but maybe I'm missing something :)

you're doing a very good job explaining me all this suspense stuff, so thank you for that 🙏

@lubieowoce
Copy link

lubieowoce commented Nov 12, 2021

not sure how useQueries would do that in a mixed array that the non-suspense things start to query while the others are suspending ?

idk, i wasn't imagining anything fancy, just something in the vein of

if (defaultedOptions.some((q) => q.suspense) {
  const willSuspend = ...
  if (willSuspend) {
    // kick off the ones that don't use suspense
    startFetching(defaultedOptions.filter((q) => !q.suspense))
  }
  // suspend on the ones that *do* use suspense
  throw fetchAll(defaultedOptions.filter((q) => q.suspense));
}

It's also a question of "who does this?", and "is this really a use-case"? Why would you suspend one query but not the other?

well... i would like to do it, sometimes 😁 Here's an example of when you might want this, a variation on the criticalData/niceToHaveData pattern from above:

const Page = () => (
  <div>
    <h1>FooHub</h1>
    <Suspense fallback={<UserWidgetSkeleton />}
      <UserWidget />
    </Suspense>
  </div>
);

const UserWidget = () => {
  const [
    { data: profile },
    repos,
  ] = useQueries([
    { ...profileQuery, suspense: true },
    { ...reposQuery, suspense: false },
  ]);
  return (
    <div>
      <img src={profile.avatarUrl} />
      <h2>@{profile.username}</h1>
      <div>Joined {profile.joinedDate}</div>
      <div>
        {repos.isLoading
          ? <Spinner />
          : `this user has ${repos.data.length} repositories`
        }
      </div>
    </div>
  );
}

The gist is, we can't really show anything until profileQuery is ready, so Suspense works well. But if profileQuery is ready, we want to render a profile-thingy, with a small spinner at the bottom until reposQuery is done.

Of course, you could get around this by preloading stuff on Page, which probably should be happening anyway... but I'm not sure if that means we shouldn't support this pattern. Maybe it does?

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 12, 2021

The gist is, we can't really show anything until profileQuery is ready, so Suspense works well. But if profileQuery is ready, we want to render a profile-thingy, with a small spinner at the bottom until reposQuery is done.

that use-case also makes sense, and if you can get that done, that would be indeed cool.

Here is a quick example with a suspense query and a non-suspense query:

https://codesandbox.io/s/react-query-starting-point-forked-j8qml?file=/src/App.tsx

You can see that the suspense query loads first, and only then the non-suspense query starts to fetch. They don't run in parallel. I think this is in line with what you outlined previously.

We currently subscribe in a useEffect, which is why the subscription doesn't trigger because suspense suspends before that. With React 18, this will change to useSyncExternalStorage, but I'm not entirely sure if that will change anything in this regard.

If I get it right, your suggestion is to call fetchOptimistic on all queries during render if at least one query has suspense enabled, but only throw those that want suspense? I'm not sure if that works, or if thats legal by the rules of react, or if that won't trigger another background-refetch (given staleTime of 0) once the suspension ends. But I think you can explore that if you want :)


all in all, even if we decide to go that route and keep suspense and useErrorBoundary as normal option within each query, maybe we might come up with options that we want to apply to all queries. I can't think of any now 😅 so we can also defer that decision :)

I just think that the api would be more extensible and future proof if it were an object

@lubieowoce
Copy link

lubieowoce commented Nov 13, 2021

If I get it right, your suggestion is to call fetchOptimistic on all queries during render if at least one query has suspense enabled, but only throw those that want suspense?

yeah, that's more or less what I'm thinking!

I'm not sure if that works, or if thats legal by the rules of react

I hope i can make it work :) and AFAIU, it's fine to do side-effecty stuff like this as long as you suspend afterwards and take care to not do it again if it's already done. reminds me of the (officially supported!) one-time-setstate init pattern:

const [thingy, setThingy] = useState();
if (!thingy) {
  setThingy(new Thingy(...));
}

or if that won't trigger another background-refetch (given staleTime of 0) once the suspension ends.

yeah, refetchOnMount has proved a bit problematic in my attempts at a suspenseful useQueries even without considering the mixed-suspense case. i'm still figuring that out. i imagine we're going to have to overload the meaning of {...}OnMount to cover suspense mode, where it happens on first render, and we don't want it to happen again on mount. AFAICT useQuery does a little workaround for this by setting a default staleTime: 1000 but I don't think that scales to multiple queries, as they could take arbitrary long and thus some of them could still become "stale" by the time we mount.

@lubieowoce
Copy link

lubieowoce commented Nov 13, 2021

the API itself (putting aside all the suspense stuff)

I just think that the api would be more extensible and future proof if it were an object

what if we did the same thing we do with query options, and allow two forms:

useQueries([A, B, C])
useQueries({ queries: [A, B, C], someFutureSetting: ... })

or maybe a mix, where there's an extra param - trivially backwards compatible!

useQueries([A, B, C], { someFutureSetting: ... })

tbh where i'm coming from is i just really don't want to type { queries: [...] } everywhere if there's no need to 😁

but it's true, an object lets us do more stuff! some very handwavy examples i came up with:

// #1. Named queries
// (technically could be done without the outer object,
// but then we'd be dealing with name conflicts between
// "hoisted" settings and queries.
// and we'd need "reserved names" to avoid conflicts,
// which always sucks and becomes a back-compat pain)
const { a, b } = useQueries({
  queries: { a: A, b: B, c: C },
});

// #2. Multiple arrays
// (idk what `lazyQueries` would do, just an example)
useQueries({
  queries: [A, B, C],
  lazyQueries: [D, E],
});

// #3. a Promise.all sort of thing
const aPlusB = useQueries({
  queries: [A, B],
  combine: ([a, b]) => a + b,
})

// #4. multiple queries helper?
// equivalent to
//  useQueries([idA, idB].map((id) =>
//    ({ key: ['thing', id], queryFn: ... })
//  ))
// but maybe there's something smart we can do with
// the knowledge that they're all "instances of the same query"?
const aPlusB = useQueries({
  values: [idA, idB],
  keyFn: (id) => ['thing', id],
  queryFn: (id) => fetch(`/api/get-thing/${id}`),
})

(these are just quick examples of the possibilities and not necessarily APIs i'd want -- some of them stray firmly into DSL-land, which i'm not a fan of because those tend to grow until they're a bad little scripting language. if we wanted "query combinators" like that combine example, it'd probably be better to start with splitting useQuer{y,ies} into define-a-resource and use-a-resource a la Relay's useQueryLoader + usePreloadedQuery and work from there)

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 13, 2021

what if we did the same thing we do with query options, and allow two forms:

in v4, we'll also remove all overloads and provide only one streamlined api for everything - an object.

so yeah, with this in mind alone, I think it makes sense to change the api to accept an object, where the current array then lives under queries.

but it's true, an object lets us do more stuff! some very handwavy examples i came up with:

yes, these are all good examples of things we can do in the future once we have the new api.


I'll add the array -> object change to #2918 because we'll very likely also add a codemod that does all this. We can keep this issue open to track if / which options should be moved to the top level. If we can't make mixed suspense work, this would be one candidate. Otherwise, if we can support all options on each query, we don't need to do anything further and just have the option of adding more options (pun intended) in the future :)

@lubieowoce
Copy link

in v4, we'll also remove all overloads and provide only one streamlined api for everything - an object.
so yeah, with this in mind alone, I think it makes sense to change the api to accept an object, where the current array then lives under queries.

Ohhh not sure how I missed that... in that case an object will be consistent with the rest, so it makes sense! 👍

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 26, 2021

@lubieowoce any updates on making useQueries work with suspense?

@lubieowoce
Copy link

lubieowoce commented Nov 26, 2021

@TkDodo work has stalled a bit, i've been busy 😅 def not PR ready yet + there's probably a bunch of junk i left lying around, but the WIP is here: https://github.com/lubieowoce/react-query/tree/feature/use-queries-suspense-2
i also have some uncommitted changes trying to clean up the bits that throw promises/errors - i'm pretty sure the code from the original PR would suspend in a case where it shouldn't.

TBH i also think i got a bit overwhelmed - the codebase has a lot of moving pieces. i could use some guidance on how it's all meant to fit together... maybe a chat somewhere w/ a shorter feedback loop than GH comments if you've got time?


also this reminds me of a v4-adjacent thing to consider. the name "refetchOnMount" doesn't make a lot of sense in suspense mode, because the component might suspend (& start fetching) before mount... nothing better comes to mind rn but maybe there's a better name that encompasses both modes?

@TkDodo
Copy link
Collaborator Author

TkDodo commented Dec 3, 2021

@lubieowoce we can definitely have a chat if you want - feel free to reach out to me on the TanStack Discord or a Twitter DM.

also this reminds me of a v4-adjacent thing to consider. the name "refetchOnMount" doesn't make a lot of sense in suspense mode, because the component might suspend (& start fetching) before mount... nothing better comes to mind rn but maybe there's a better name that encompasses both modes?

I think the component mounts, in which case we fetch, and then it suspends. It will then re-mount, which is why we actually set a small staleTime when using suspense to avoid a constant re-fetching. I think the name is still accurate for these cases, technically :)

@lubieowoce
Copy link

lubieowoce commented Dec 3, 2021

@TkDodo

feel free to reach out to me on the TanStack Discord or a Twitter DM.

alright, will do! thanks :)

I think the component mounts, in which case we fetch, and then it suspends. It will then re-mount, which is why we actually set a small staleTime when using suspense to avoid a constant re-fetching. I think the name is still accurate for these cases, technically :)

removed some incorrect stuff

hmm. i can't remember now, but IIRC useQuery won't suspend if the fetch is a refetch, it'll just show stale data until the refetch completes...? in that case agreed, need to change it, because that'd really happen on mount. but if it suspends when refetching, that might happen during the first render (and interrupt it), in which case the on mount bit becomes a bit misleading.

(really hope i'm not confusing sth about how refetchOnMount works cause that'd make me look like a huge dummy!)

@TkDodo
Copy link
Collaborator Author

TkDodo commented Dec 4, 2021

hmm. i can't remember now, but IIRC useQuery won't suspend if the fetch is a refetch, it'll just show stale data until the refetch completes...? in that case agreed, need to change it, because that'd really happen on mount. but if it suspends when refetching, that might happen during the first render (and interrupt it), in which case the on mount bit becomes a bit misleading.

no, it does not suspend on refetches. That could happen on every window focus and would show loading spinners all over the place, which is something react-query is good at avoiding 😅 . Only the hard loading state is handled by suspense.

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

No branches or pull requests

3 participants