Closed srikrsna-buf closed 9 months ago
I like the suggested approach from a tree shaking perspective, as well as removing more more required plugins. Migration is a concern but, as you mentioned, we are pre v1, so if there's any time to do it, it's now. The suggested API looks like it shares a little with the experimental plugin's DX but without the problem of creating too many APIs in order to support tanstack/query@v5.
That said, this new approach would mean we'd have to wrap tanstack/query. I think we can get away with peerDependencies but it'll definitely need some examination and experimentation to see how that works out in practice.
I am in favour of exploring in this direction.
So far, I'm pretty encouraged by my exploration down this direction. It certainly simplifies the library by quite a bit. As expected, the API is very different from the original API but initial thoughts are it's far less verbose and more approachable. The one thing we'll need (as mentioned) is an additional API in @connectrpc/connect
's createConnectRouter
that supports passing in a single method descriptor. The type we'd want looks like:
import type { Message, MethodInfoUnary } from "@bufbuild/protobuf";
/** Defines a standalone method and associated service. TBD the proper name for this thing. */
export type MethodDescriptorUnary<
I extends Message<I>,
O extends Message<O>,
> = MethodInfoUnary<I, O> & {
localName: string;
service: {
typeName: string;
};
};
Ideally, the ConnectRouter.rpc
method could accept a single one of those as well as the service + method. That should enable us to leverage this across all the testing/mocking libraries for connect.
Not exactly sure what we'd want to call this type. I'm open to suggestions.
Side note, this also makes it easier for us to separate our streaming methods to their own dedicated hooks + types (whenever we figure out a proper way of handling those.)
@srikrsna-buf I've published a PR #224 with a tentative implementation (minus docs). Let me know if it looks good so far.
The user for facing API today looks like:
The generated code is as follows:
Issues with the above API and generated code:
useQuery
twice is verbose and confusing, if one has to set other fields that the query APIs accept it gets even more verbose with a slightly different looking call:useQuery({example.useQuery({})..., extra: value})
Possible solution
I propose a framework agnostic plugin accompanied by a new react specific API that wraps the original hooks. The generated code will look like:
This solves the tree-shaking problem by just generating method specific definitions of the rpcs. This is also framework agnostic as it only generates the rpc definitions without any imports of query APIs.
The runtime API will take the form:
In this API, the second parameter is for the request, the optional third is to provide additional parameters to the underlying Hooks API. This way we can only expose safe to override properties. This will also make wrapping the hooks to modify the types easier (https://github.com/connectrpc/connect-es/issues/694#issuecomment-1677929961). We should also expose supplementary API for things like calculating query keys and other bits that help us construct the hooks.
Along with this we should expose a mocking wrapper similar to
createRouterTransport
that is rpc oriented where users can import the connect-query generated constants to mock rpcs.With this structure contributors only need to create the runtime package and not bother about the plugin.
Implementation
This is a breaking change and a significant shift from our current API. I am not entirely sure if we can write a migrator but even if we did it won't be trivial.
One option is to hold off on #219 and #179 and release a parallel package for Tanstack v5. We continue to support the current version for Tanstack v4 and users wanting to upgrade to Tanstack v5 can update to the new react specific package. This gives us some time to work on the migrator and at the same time new users will start using the new APIs.
@paul-sachs @timostamm thoughts?