V1 API: Wrapping react-query

[paul-sachs]

Introducing a whole new API for connect-query. This API ties itself more tightly with the fantastic @tanstack/react-query in order to improve developer experience and reduce bundle size.

At the core of this change is the new MethodUnaryDescriptor type, which is essentially just a self contained description of a service method. The new code generator just outputs one of these per method and those are passed to the hooks/methods. This new additional type brings a huge level of flexibility about how we write additional methods, and it's trivial to write your own hooks/methods around these simple types.

Example usage

import { useQuery } from "@connectrpc/connect-react-query";
import { say } from "./gen/eliza-ElizaService_connectquery";

...

const { data } = useQuery(say);

Breaking changes

• use*Query hooks now return data instead of just options.
• Many create* methods have been removed.
• protoc-gen-connect-query now outputs very simple method descriptors instead of a bunch of hooks per method.

Reasoning

There are a number of reasons we've decided to make this major breaking change.

1. The API surface is much smaller, leading to less confusion about how to use each method.
2. Smaller core code size and more modular organization leads to better tree shaking (since each generated method doesn't come along with a generated hook).
3. Package names are now explicit about their dependencies, making it easier to develop other packages based on these packages/generated code.
4. More tightly integrating with @tanstack/react-query@5 provides better usage of Suspense versions of the API.
5. New generated code is easier to expand and build your own custom hooks for.

Migration

The migration process is easy but manual:

Before

import { getUserOrganization } from "@apigen/org/alpha/registry/v1alpha1/organization-OrganizationService_connectquery";
import { useQuery } from "@tanstack/react-query";

...

const getUserOrganizationQuery = useQuery({
    ...getUserOrganization.useQuery({
      userId: currentUser?.id,
      organizationId,
    }),
    useErrorBoundary: false,
    enabled: currentUser !== null,
});

After

import { getUserOrganization } from "@apigen/org/alpha/registry/v1alpha1/organization-OrganizationService_connectquery";
import { useQuery } from "@connectrpc/connect-react-query";

...

const getUserOrganizationQuery = useQuery(getUserOrganization, {
      userId: currentUser?.id,
      organizationId,
    },
    {
      useErrorBoundary: false,
      enabled: currentUser !== null
    }
});

Fixes #220

[srikrsna-buf]

API looks great! Left some questions

[srikrsna-buf]

I don't think we need the service here line 49 to 67 can be deleted

[paul-sachs]

I left this for ease of writing tests. Ideally, we'd remove this before we actually merge it, assuming we're happy with the method descriptor.

[srikrsna-buf]

Do we need to export the create*Options APIs?

[paul-sachs]

Yes, these are needed when used with useQueries({ queries: [ createUseQueryOptions(...) ] }) in order to define parallel queries. I thought perhaps we could define our own useQueries() but that would mean it would be impossible to mix connect queries with other external queries.

[srikrsna-buf]

@tannerlinsley It'd be great to get your inputs on the new API. I think you also had feedback regarding what the hooks accept as input for the rpc requests.

[paul-sachs]

@srikrsna-buf @timostamm Alright, i've updated the readme for a pass at documentation (update to doc page will obviously need to wait). I wanted to gauge a few ideas about release.

• protoc-gen-connect-query: This plugin is now not even really connect-query related and is really just spitting out generic method descriptors. We can keep it like that for now, but if we create any other tools based on those descriptors, it probably makes sense to move this plugin elsewhere and rename it. We can obviously punt that decision till later but I wanted to raise it now.
• connect-react-query: really just a rename and a v1 all at the same time. I thought it made sense to do the rename here as well as the v1 since both can be considered breaking so might as well make it a single breaking change.

[srikrsna-buf]

Looks great! Just had one suggestion:

I think we should expose functions to call an rpc (unary to begin with) that accept a transport and the request in a type safe way. Then users will never need to interact with connect-web and more importantly protoc-gen-connect-es and can directly call rpcs. This is will more or less look like the unary query function.

If we expose this function then we can also un export the create* functions reducing the API surface significantly.

[timostamm]

Thanks for the updates to the docs!

Krishna's suggestion seems to be worthwhile to me.

[paul-sachs]

@srikrsna-buf hmm, I see what you mean. So instead of having something link:

const [result1, result2] = useQueries({
  queries: [
    createUseQueryOptions(serviceMethod1, input1, options1),
    createUseQueryOptions(serviceMethod2, input2, options2)
  ]
});

We'd just document/encourage the following:

const [result1, result2] = useQueries({
  queries: [
    { queryKey: createConnectQueryKey(serviceMethod1, input1), queryFn: createConnectQueryFn(serviceMethod1, input1, options1) },
    { queryKey: createConnectQueryKey(serviceMethod2, input2), queryFn: createConnectQueryFn(serviceMethod2, input2, options2) },
  ]
});

I find that a little more error prone myself (due to potential mismatches between queryKey and queryFn) but it does admittedly reduce the API surface layer. We'd still need another function for infiniteQueries since the key generated for that is actually slightly different than our default key (since pageParam impacts the key).

Perhaps we can simplify the exported options a little bit though.The difference between suspense/non-suspense APIs are entirely within the options they take, to make it consistent with other APIs (like useQuery/useSuspenseQuery). Perhaps a function similar to our old API that just returns queryKey and queryFn would suffice, allowing the user to add other options as they desire (onError, onSuccess, etc) by just spreading the result. That would at least remove two APIs , createUseSuspenseInfiniteQueryOptions and createUseSuspenseQueryOptions.

[paul-sachs]

@srikrsna-buf @timostamm updated docs and API to remove the dedicated createUseSuspense*Options APIs. I think this is as tiny an API I can get while still allowing for full access to typesafe react-query.

[srikrsna-buf]

Chatted offline, decided to reduce API surface further by removing all the create*Options functions. Exposed a function call rpcs directly (unary for now). We see this potentially moving to @connectrpc/connect.

[srikrsna-buf]

Looks good to me!

Maybe @timostamm has an opinion about what the call function should look like but we can revisit that part, so stamping for now

[timostamm]

Really nice, let's get this merged!

But the PR title "Experimental API suggestion" is not descriptive. The title needs to summarize what this does.

Also:

1. When this is released, we need to explain crystal clear what is changing, why, and what users need to do. IMO it's best to do that right in the PR description, but I'm fine doing it just in the release notes if you prefer.

2. Does it make sense to rename the package @connectrpc/connect-query to @connectrpc/connect-react-query in this PR? Splitting it out into a separate change would make it quite a bite easier to follow what's going on.

[paul-sachs]

@timostamm i will definitely update the pr description to layout all the changes and why. Plus some migration examples.

As for your 2nd point, I was originally thinking that this plug-in was no longer react specific so it could be used for any version of connect-query but since that doesn't exist yet, perhaps that's optimizing for a later case that's not important. I would be down with renaming the plug-in.

[paul-sachs]

My one argument for not renaming the plugin, is the ending of the file get's pretty long, so it'll generate things like: ./gen/eliza-ElizaService_connectreactquery which does feel awkward.

[timostamm]

I can't follow. The plugin name doesn't change, from what I can see.

[paul-sachs]

@timostamm oh, i definitely misread, sorry. Yeah I can see why moving the rename to a different PR would make sense. I'll update.

[paul-sachs]

@timostamm alright, removed package rename (which we'll discuss separately offline).