TanStack / query

🤖 Powerful asynchronous state management, server-state utilities and data fetching for the web. TS/JS, React Query, Solid Query, Svelte Query and Vue Query.
https://tanstack.com/query
MIT License
40.54k stars 2.73k forks source link

Support the ability to provide a context #2548

Closed blackarctic closed 2 years ago

blackarctic commented 2 years ago

This adds support for passing in a context as other robust libraries (such as Redux) support: https://react-redux.js.org/api/provider#props.

This is important as right now React Query supports creating multiple queryClient instances but does not currently support the ability to specify which queryClient instance should be used with useQueryClient. In the case of nested QueryClientProvider instances in an application, the ability to specify which context (and thus which QueryClientProvider and queryClient) should be used is critical. This adds such capability.

These changes should be 100% backwards compatible.

vercel[bot] commented 2 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tanstack/react-query/GEfLtbDQcDS1CVF19GfEAwiYdLnf
✅ Preview: https://react-query-git-fork-blackarctic-support-passin-783825-tanstack.vercel.app

codesandbox-ci[bot] commented 2 years ago

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 3c788dba520794e482bd748e2906420cbad7d34e:

Sandbox Source
tannerlinsley/react-query Configuration
tannerlinsley/react-query: basic-typescript Configuration
TkDodo commented 2 years ago

What would be the use-case for needing multiple, nested QueryClientProviders where you do not want to lookup the nearest one?

blackarctic commented 2 years ago

What would be the use-case for needing multiple, nested QueryClientProviders where you do not want to lookup the nearest one?

The scenario I have been seeing is an app container that wants to provide some core data using React Query but wants to have no coupling to the apps that it renders that may or may not be using their own React Query for their own data. In that case, trying to access the core data will try to access the app’s React Query instead of the app container’s.

This becomes an issue of isolation where a component (whether that be an app, a button, or anywhere inbetween) desires isolation from wherever it is used.

Assuming a single context for all React Query instances leads to this issue and is the reason robust libraries like Redux support the option to specify which context to use at the provider and at the consumer level.

TkDodo commented 2 years ago

I think the code changes look fine, I'd still want @tannerlinsley's opinion here. We'd also need some tests and updates to the docs please. Feel free to wait with that until Tanner gives his okay :)

blackarctic commented 2 years ago

Any updates here?

TkDodo commented 2 years ago

@tannerlinsley what do you think please? Is that a feature that should make it into the library?

blackarctic commented 2 years ago

To illustrate the need for this a bit more clearly here:

import { ContainerDataProvider, useUser } from "@my-scope/container-data";
import { AppDataProvider } from "@my-scope/app-data";
import { MyComponentDataProvider, useItems } from "@my-scope/my-component-data";

<ContainerDataProvider> // <-- Provides container data using its own React Query provider
  ...
  <AppDataProvider> // <-- Provides app data using its own React Query provider
    ...
      <MyComponentDataProvider> // <-- Provides component data using its own React Query provider
        <MyComponent />
      </MyComponentDataProvider>
    ...
  </AppDataProvider>
  ...
</ContainerDataProvider>

// Example of hooks provided by the "DataProvider" components above:
const MyComponent = () => {
  const user = useUser(); // <-- Would use the context specified in ContainerDataProvider. FAILS currently because we cannot specify context.
  const items = useItems(); // <-- Would use the context specified in MyComponentDataProvider
  ...
}

Each "DataProvider" uses its own React Query provider for isolation and abstraction. Those that consume the data of these providers should not have to know what library they are using for data fetching and storage.

In this case, MyComponent needs to access useQuery from both MyComponentDataProvider and ContainerDataProvider. Unless we can specify which context we wish to use for each useQuery, it will default to MyComponentDataProvider and fail.

tannerlinsley commented 2 years ago

I think this is a valid addition as long as:

TkDodo commented 2 years ago

@blackarctic would you like to continue with the PR now that we have the approval from Tanner?

blackarctic commented 2 years ago

Yes. Sounds like we need to:

TkDodo commented 2 years ago

please let me know when this is ready for re-review, thanks.

blackarctic commented 2 years ago

@TkDodo PR should be ready for re-review

codecov[bot] commented 2 years ago

Codecov Report

Merging #2548 (3c788db) into alpha (1b91f3f) will increase coverage by 0.48%. The diff coverage is 96.29%.

@@            Coverage Diff             @@
##            alpha    #2548      +/-   ##
==========================================
+ Coverage   96.85%   97.33%   +0.48%     
==========================================
  Files          44       44              
  Lines        2318     2326       +8     
  Branches      671      681      +10     
==========================================
+ Hits         2245     2264      +19     
+ Misses         70       60      -10     
+ Partials        3        2       -1     
Impacted Files Coverage Δ
src/core/hydration.ts 100.00% <ø> (ø)
src/devtools/devtools.tsx 86.93% <66.66%> (+5.03%) :arrow_up:
src/core/utils.ts 100.00% <100.00%> (ø)
src/devtools/tests/utils.tsx 95.00% <100.00%> (+0.26%) :arrow_up:
src/reactjs/Hydrate.tsx 100.00% <100.00%> (ø)
src/reactjs/QueryClientProvider.tsx 100.00% <100.00%> (ø)
src/reactjs/tests/utils.tsx 97.72% <100.00%> (+0.05%) :arrow_up:
src/reactjs/useBaseQuery.ts 100.00% <100.00%> (ø)
src/reactjs/useIsFetching.ts 100.00% <100.00%> (ø)
src/reactjs/useIsMutating.ts 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1b91f3f...3c788db. Read the comment docs.

TkDodo commented 2 years ago

code looks good - there seems to be one test failing consistently.

blackarctic commented 2 years ago

Not seeing any failing tests but it looks like coverage dipped by 0.03% in src/devtools/devtools.tsx. However, none of the uncovered lines of code were touched as part of this PR. Trying to get this PR in, though, so I added a test to test some of the untested code in the file to get the coverage up.

Screen Shot 2021-11-29 at 5 28 31 PM
TkDodo commented 2 years ago

Thank you. I am thinking that maybe we should wait with the PR until v4 though. We are planning to change the api for useQueries to become an object, which would fit this use-case very well, because we could provide the context option at the very top, working for all queries:

useQueries({
  queries: [query1, query2, ..,. queryN],
  context: MyContext
})
blackarctic commented 2 years ago

Thank you. I am thinking that maybe we should wait with the PR until v4 though. We are planning to change the api for useQueries to become an object, which would fit this use-case very well, because we could provide the context option at the very top, working for all queries:

useQueries({
  queries: [query1, query2, ..,. queryN],
  context: MyContext
})

That would mean each query config (query1, query2, ..,. queryN) could not contain their own context? I would prefer to do that as a followup as that changes a good amount of types and implementation.

Is it typical to keep a ready PR open? I am worried about accumulating conflicts and/or bugs.

TkDodo commented 2 years ago

Judging from the current implementation, I was under the impression that it is a requirement that each query in useQueries uses the same context?

blackarctic commented 2 years ago

Judging from the current implementation, I was under the impression that it is a requirement that each query in useQueries uses the same context?

Correct, but this now changes the types of these query objects so that they cannot include the context option. They are now a different type of query object than what is passed to useQuery. This also changes the implementation of useQueries since we can no longer blindly pass this different query object (without the context) to handlers of single queries because those might be handling the context. This will require us to re-add the context to each query object before passing it to any single query handlers.

TkDodo commented 2 years ago

Correct, but this now changes the types of these query objects so that they cannot include the context option. They are now a different type of query object than what is passed to useQuery.

I think that's fine - we can Omit the context on type level for useQueries.

This also changes the implementation of useQueries since we can no longer blindly pass this different query object (without the context) to handlers of single queries because those might be handling the context. This will require us to re-add the context to each query object before passing it to any single query handlers.

yes, we would need to spread the context from the top level into each of the queries. Is that a problem?

blackarctic commented 2 years ago

Correct, but this now changes the types of these query objects so that they cannot include the context option. They are now a different type of query object than what is passed to useQuery.

I think that's fine - we can Omit the context on type level for useQueries.

This also changes the implementation of useQueries since we can no longer blindly pass this different query object (without the context) to handlers of single queries because those might be handling the context. This will require us to re-add the context to each query object before passing it to any single query handlers.

yes, we would need to spread the context from the top level into each of the queries. Is that a problem?

Nope, not a problem. Just outlining the additional changes that will need to happen and the increased risk of issues in long-running PRs.

TkDodo commented 2 years ago

I've just merged the new useQueries api into the alpha branch. can you please base your PR on alpha, thank you :)

TkDodo commented 2 years ago

This looks very good, thank you 🙏 . It seems that the useQueryClient api docs do not mention the new context yet, could we add that there as well?

Also, I'm a bit worried that the name context might be confused with the queryFunctionContext that we already have - the object that is injected into the queryFn. Do you think it would be better to choose a different name?

blackarctic commented 2 years ago

This looks very good, thank you 🙏 . It seems that the useQueryClient api docs do not mention the new context yet, could we add that there as well?

Also, I'm a bit worried that the name context might be confused with the queryFunctionContext that we already have - the object that is injected into the queryFn. Do you think it would be better to choose a different name?

Hmmm, I see your point. Well we could call it reactContext, reactQueryContext, customReactQueryContext, customReactContext, customReactQueryContext, queryContext, or customQueryContext? Any suggestions?

TkDodo commented 2 years ago

can you also add something to the New Features section in the migrating to v4 doc please. I think it's good to let people know that way that we now have this feature 🚀

bertho-zero commented 2 years ago

Is it possible to export the default context or getQueryClientContext too? I have the same structure as https://github.com/tannerlinsley/react-query/pull/2548#issuecomment-915496288 but I don't necessarily need to pass the context option everywhere.

I would like to use the parent context if it exists, much like contextSharing but without using window, the idea is to do something like:

function SubApp({}) {
  // const parentQueryClient = useQueryClient(); // Does not work because throws an error if tested in isolation
  const parentQueryClient = useContext(QueryClientContext);
  const queryClient = useMemo(() => (parentQueryClient || new QueryClient()), [parentQueryClient]);

  return (
    <QueryClientProvider client={queryClient}>
      {/* ... */}
    </QueryClientProvider>
  );
}

Or just an option that skips the error from useQueryClient.

TkDodo commented 2 years ago

@blackarctic we are nearing the release of v4. is there a chance we could get this in?

blackarctic commented 2 years ago

Is it possible to export the default context or getQueryClientContext too? I have the same structure as #2548 (comment) but I don't necessarily need to pass the context option everywhere.

I would like to use the parent context if it exists, much like contextSharing but without using window, the idea is to do something like:

function SubApp({}) {
  // const parentQueryClient = useQueryClient(); // Does not work because throws an error if tested in isolation
  const parentQueryClient = useContext(QueryClientContext);
  const queryClient = useMemo(() => (parentQueryClient || new QueryClient()), [parentQueryClient]);

  return (
    <QueryClientProvider client={queryClient}>
      {/* ... */}
    </QueryClientProvider>
  );
}

Or just an option that skips the error from useQueryClient.

Exported the defaultContext from QueryClientProvider. @TkDodo Is that ok with you?

blackarctic commented 2 years ago

can you also add something to the New Features section in the migrating to v4 doc please. I think it's good to let people know that way that we now have this feature 🚀

Done

blackarctic commented 2 years ago

@blackarctic we are nearing the release of v4. is there a chance we could get this in?

@TkDodo Sorry for the delay. Should be good to re-review.

TkDodo commented 2 years ago

thank you so much for bearing with me here @blackarctic. It's been a long 7 months since you opened the PR. It will be a great addition to v4 🚀 . Thank you for the contribution 🙇

tannerlinsley commented 2 years ago

:tada: This PR is included in version 4.0.0-alpha.21 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

tannerlinsley commented 2 years ago

:tada: This PR is included in version 4.0.0-beta.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: