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

Availability of createUseQueryOptions and createUseInfiniteQueryOptions in v1 #296

Closed
nakker1218 opened this issue Dec 13, 2023 · 12 comments · Fixed by #343
Closed

Availability of createUseQueryOptions and createUseInfiniteQueryOptions in v1 #296

nakker1218 opened this issue Dec 13, 2023 · 12 comments · Fixed by #343

Comments

@nakker1218
Copy link

nakker1218 commented Dec 13, 2023

Is the absence of createUseQueryOptions and createUseInfiniteQueryOptions in v1 intentional? These methods appear to be extremely important when React context is not available.

https://github.com/connectrpc/connect-query-es/blob/main/packages/connect-query/src/index.ts#L19-L24

The README mentions createUseQueryOptions:

What if I have a custom Transport?
If the Transport attached to React Context via the TransportProvider isn't working for you, then you can override transport at every level. For example, you can pass a custom transport directly to the lowest-level API like useQuery or createUseQueryOptions.

Does this only work with React?
Connect-Query does require React, but the core (createUseQueryOptions) is not React specific so splitting off a connect-solid-query is possible.

(README Link: https://github.com/connectrpc/connect-query-es?tab=readme-ov-file#does-this-only-work-with-react)

Is there an alternative method now, or was their removal unintentional? If it's an unintentional, I'm willing to help with a PR.

@paul-sachs
Copy link
Collaborator

This was an intentional change to reduce the API surface and create a more focused API. That said, I would be open to exposing these APIs again given a specific use case that can't be served otherwise.

@paul-sachs
Copy link
Collaborator

We did expose a helper, callUnaryMethod which can often be used similarly, just without being specific to infiniteQuery or query

@paul-sachs
Copy link
Collaborator

I guess I also forgot to mention createConnectQueryKey(), which provides the queryKey.

@nakker1218
Copy link
Author

Thank you for your response. I understand now that it was an intentional change. First, we will try using callUnaryMethod in our product and see if there are any issues. For now, I will close this issue and if anything comes up, we will create a new one!

@lsmurray
Copy link

I think it would be nice if these were exposed. They are useful for building custom hooks but also for some APIs such as queryClient.ensureQueryData

    await queryClient.ensureQueryData(
      createUseQueryOptions(
        rpc,
        {
           input
        },
        {
          transport,
        }
      )
    );

While I could implement this with the unary helpers it ends up being the same code over and over again and the code is basically a copy paste of the internal API.

@paul-sachs
Copy link
Collaborator

In our codebase, this kind of pattern arose too, but we ended up going down the path of a custom QueryClient, which provided typesafe variants of the APIs, so invalidateQueries became invalidateConnectQueries with appropriate args. Ditto with setConnectQueryData. It's still a little early within our codebase to publish it, but I personally think it provides a better DX and could even eliminate the need (but keep an option for) the transport.

Obviously one does not negate the other but in the spirit of reducing the number of ways to do things, the custom client feels more directed and adds more value than just removing helper code.

If that would be of interest to you, I can open a tracking issue for that work and we can get a little feedback from the ecosystem in a slightly more visible way.

@lsmurray
Copy link

lsmurray commented Feb 1, 2024

That sounds slightly better than what I implemented but similar. Would be happy to provide feedback when you're ready to publish anything.

@advdv
Copy link

advdv commented Feb 19, 2024

I'm in the situation that I want to call "context.queryClient.ensureQueryData" (in Tanstack Router's loader as described here https://tanstack.com/router/latest/docs/framework/react/guide/external-data-loading#a-more-realistic-example-using-tanstack-query) But I'm a bit confused as to what the recommendation is right now?

Do I extend the query client? But I'm not sure how to do this, is there an example? I've tried to copy the createUseQueryOptions from the codebase but it is quite a lot it seems with all the required internal types, or maybe 'm doing it wrong as well?

Also, if i just do something like this:

import { createFileRoute } from "@tanstack/react-router";
import { say } from "@buf/connectrpc_eliza.connectrpc_query-es/connectrpc/eliza/v1/eliza-ElizaService_connectquery";
import {
  callUnaryMethod,
  createConnectQueryKey,
} from "@connectrpc/connect-query";
import { queryOptions, useSuspenseQuery } from "@tanstack/react-query";
import { connectTransport } from "@/api";

// query options
const sayQueryOptions = queryOptions({
  // eslint-disable-next-line @tanstack/query/exhaustive-deps
  queryKey: createConnectQueryKey(say, { sentence: "hello" }),
  queryFn: () =>
    callUnaryMethod(
      say,
      { sentence: "hello" },
      { transport: connectTransport },
    ),
});

// route
export const Route = createFileRoute("/about/")({
  component: About,
  loader: ({ context }) => context.queryClient.ensureQueryData(sayQueryOptions),
});

// page component
function About() {
  const { data } = useSuspenseQuery(sayQueryOptions);

  return <div className="p-2">Hello: {data?.sentence}</div>;
}

the https://tanstack.com/query/latest/docs/eslint/exhaustive-deps listing rule complains. Also giving me the feeling I'm doing something wrong.

Imo it's probably pretty common to use Tanstack query in router data "loaders" where the react context is not available.

@paul-sachs
Copy link
Collaborator

paul-sachs commented Feb 21, 2024

So the way we are creating our own custom query client is like so:

import { QueryClient as TSQueryClient, InvalidateOptions,
  SetDataOptions, } from "@tanstack/react-query";
import {
  callUnaryMethod,
  createConnectQueryKey,
  defaultOptions,
  DisableQuery,
  disableQuery,
  TransportProvider,
  useInfiniteQuery,
  useMutation,
  createProtobufSafeUpdater,
  useTransport,
  ConnectQueryKey
} from "@connectrpc/connect-query";
import { CallOptions, ConnectError, Transport } from "@connectrpc/connect";
import {
  AnyMessage,
  Message,
  MethodInfoUnary,
  PartialMessage,
  ServiceType,
} from "@bufbuild/protobuf";

export class QueryClient extends TSQueryClient {
  invalidateConnectQueries<I extends Message<I>, O extends Message<O>>(
    methodSig: MethodSignature<I, O>,
    input?: PartialMessage<I>,
    options?: InvalidateOptions
  ) {
    return this.invalidateQueries(
      {
        queryKey: createConnectQueryKey(methodSig, input),
      },
      options
    );
  }

  setConnectQueryData<I extends Message<I>, O extends Message<O>>(
    methodSig: MethodSignature<I, O>,
    updater: ConnectUpdater<O>,
    input?: typeof disableQuery | PartialMessage<I> | undefined,
    options?: SetDataOptions | undefined
  ) {
    return this.setQueryData(
      createConnectQueryKey(methodSig, input),
      createProtobufSafeUpdater(methodSig, updater),
      options
    );
  }
}

As for the eslint rule @tanstack/query/exhaustive-deps, it's a valid point. Currently we don't use the transport as part of the key, which means that if the transport changes then related data doesn't auto fetch again. Not actually sure how we'd want to solve that one, since the transport itself can't really be serialized into a string (it's a set of functions) and thus cannot be a part of the query key.

@advdv
Copy link

advdv commented Feb 21, 2024

Ah right, the query client is just a class. That make sense, thank you

For the cache key not including the transport. Am I correct that it only means that the cache key might overlap if a project uses different transports with the same method signatures in the same project?

@paul-sachs
Copy link
Collaborator

For the cache key not including the transport. Am I correct that it only means that the cache key might overlap if a project uses different transports with the same method signatures in the same project?

Yes, that is correct. I'm not sure how realistic that scenario is, but it is possible.

paul-sachs added a commit that referenced this issue Feb 27, 2024
Ideally we'd be able to create an opinionated version of `useQueries`
but due to the complexity of those recursive types and there still being
other queryClient methods that can benefit from these APIs, I propose we
expose these internal APIs as helpers.

Fixes #333
Fixes #296
@paul-sachs
Copy link
Collaborator

Fixed in https://github.com/connectrpc/connect-query-es/releases/tag/v1.3.0

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 a pull request may close this issue.

4 participants