trpc / v10-playground

tRPC v10 procedure play
https://stackblitz.com/github/trpc/v10-playground?file=src%2Fserver%2Findex.ts,src%2Fclient.ts,src%2Fserver%2Frouters%2FpostRouter.ts&view=editor
13 stars 3 forks source link

API thoughts #23

Closed colinhacks closed 2 years ago

colinhacks commented 2 years ago

Proxy 👍

I think using Proxy makes sense, though I don't understand all the concerns mentioned here https://github.com/trpc/v10-playground/pull/21. Support for it is really good now so I don't think that should be a concern. It's a pre-requisite for some of the API ideas I have below.

The router API 😐

I'm pretty ambivalent here. I like the fact that the procedure names are now object keys instead of just strings. But the new API requires an extra level of nesting, which I don't love.

trpc.router({
  queries: {
    hello: _ => ({data: "Hello"}),
    helloagain: _ => ({data: "Hello again"}),
  }
})

trpc.router()
  .query("hello", _ => ({data: "Hello"})
  .query("hello", _ => ({data: "Hello"})

More on this later.

The resolver API 😕

I don't think people like purely functional APIs. The success of Zod is a testament to this. The io-ts API is purely functional and it's really annoying to make things optional, add default values, etc. Wrapping a schema in t.optional(...) is just more difficult than appending .optional() to it.

All that to say, the trpc.resolver(trpc.zod(...)) thing just rubs me the wrong way.

trpc.router({
  queries: {
    hello: trpc.resolver(
      trpc.zod(
        z.object({
          hello: z.string(),
        }),
      ),
      (params) => {...}
    );
  }
})

If trpc.resolver was variadic and could accept an arbitrary number of potentially context-modifying middlewares, that would almost be cool enough for this API to be worth it (almost). But I'm pretty sure that's impossible. Sure, you can increase the number of inputs it can accept by just adding more and more overloads, but that's too hacky even for me.

But even if the variadic thing was possible, I find this quite a bit less pleasant to read and write than a chained version.

That said, I see what you're trying to do here. I like the idea of the input validation being "just another middleware" and I understand the limitations with the object-based API. Here's an alternative API that I quite like:

trpc.router({
  queries: {
    hello: trpc.resolver()
      .input(z.object({ test: z.string() }))
      .modifyContext(params => ({ ctx: ... })) 
      .resolve(params => ...)
  }
})

Basically you call trpc.resolver for each endpoint, and it provides you with a bunch of convenience methods for "appending" certain types of middlewares to the chain. It's easy to add additional methods (middleware types) to the Resolver object. You get auto-completion on the method names, so it's easy for users to discover all the different kinds of middleware logic they can implement. I personally really like the "pipeline" feel of it.

Optionally trpc.resolver() could accept a v9-style object-based resolver definition for users who prefer that style.

trpc.router({
  queries: {
    hello: trpc.resolver({
      input: z.object({ ... }),
      resolve(params){
        return { ... } 
      }
    })
  }
})

Though there's an API that's even better. It avoids a bunch of repetitive calls to trpc.resolver() and doesn't represent such a dramatic shift from the v9 API.

trpc.router()
  .query("hello", route => route
    .input(z.object({ test: z.string() }))
    .modifyContext(params => ({ ctx: ... })) 
    .resolve(params => ...))
  .query("helloAgain", route => route
    .input(z.object({ test: z.string() }))
    .modifyContext(params => ({ ...params.context, newKey: "hey yo" })) 
    .resolve(params => ...))

Basically this closure API is a "power user feature" for users who prefer to write pipeline-style resolvers. It's easy to continue supporting the v9 API too. I think this is pretty close to optimal.

The client API 🤔

I'm on record as disliking the current tuple-based API.

I think this proposed API is better but still doesn't feel RPC-like to me.

const greeting = await client.query(queries['post.add'], { hello: 'world' });

I think making the API worse in order to support Cmd-Click/"Go to definition" is a really bad tradeoff. If you put this to a vote on Twitter I'm guessing a vast majority of users would prefer a cleaner-looking client API to having CMD+click support. (I'm willing to be proven wrong here, but I'm very confident in this.)

As for my ideal client API, I think it should be this:

const greeting = await trpc.query.post.add({ hello: 'world' });

This is an RPC library. Any API that doesn't feel like you're "calling" the remote function is bad, bordering on unacceptable. This is roughly how the API looked in v0.x and I would be super freakin happy to see it come back. If you don't do this, I suspect an alternative to tRPC will come along with this API and end up winning.

I submitted a POC of this API here (types only, no runtime implementation): https://github.com/trpc/v10-playground/pull/22

KATT commented 2 years ago

First of all, thank you so much for taking the time to do such a thorough write-up with your take on this!

Proxy 👍

I think using Proxy makes sense, though I don't understand all the concerns mentioned here #21. Support for it is really good now so I don't think that should be a concern. It's a pre-requisite for some of the API ideas I have below.

The only reason for me currently to add proxy is to make the CMD+click DX work, but a suggestion like #22 also requires proxy of course.

The router API 😐

I'm pretty ambivalent here. I like the fact that the procedure names are now object keys instead of just strings. But the new API requires an extra level of nesting, which I don't love.

trpc.router({
  queries: {
    hello: _ => ({data: "Hello"}),
    helloagain: _ => ({data: "Hello again"}),
  }
})

trpc.router()
  .query("hello", _ => ({data: "Hello"})
  .query("hello", _ => ({data: "Hello"})

As soon as you add more than 1 query to your router, you have less typing to do, with the added (opinionated) benefit that queries are grouped with other queries and mutations with other mutations. This makes a lot more sense to me and I tried to achieve this for very long in the current API - https://github.com/trpc/trpc/pull/37

Another thing with the current API is that queries are hard/unintuitive to write, I find myself counting parentheses and curly brackets all too often. Arguably, your suggestions with the resolver API could also address this.

However, there are more added benefits of keeping this flat. Currently, each procedure added creates a new Router which TypeScript choke as the underlying complexity grows non-linearly with each added procedure - each procedure basically has it's "own instance" within the compiler even if it's flattened and unaffected at runtime.

I need to support the perf claim with better metrics, but the fact that really big routers don't even compile in the current version of tRPC should be a testament enough. Now a 1000 procedure router is slow as well and gives you feedback loops at ~10s, but at least it compiles and those TS perf issues are likely to have a lot easier solutions when it's flat.

The resolver API 😕

I don't think people like purely functional APIs. The success of Zod is a testament to this. The io-ts API is purely functional and it's really annoying to make things optional, add default values, etc. Wrapping a schema in t.optional(...) is just more difficult than appending .optional() to it.

All that to say, the trpc.resolver(trpc.zod(...)) thing just rubs me the wrong way.

trpc.router({
  queries: {
    hello: trpc.resolver(
      trpc.zod(
        z.object({
          hello: z.string(),
        }),
      ),
      (params) => {...}
    );
  }
})

If trpc.resolver was variadic and could accept an arbitrary number of potentially context-modifying middlewares, that would almost be cool enough for this API to be worth it (almost). But I'm pretty sure that's impossible. Sure, you can increase the number of inputs it can accept by just adding more and more overloads, but that's too hacky even for me.

But even if the variadic thing was possible, I find this quite a bit less pleasant to read and write than a chained version.

That said, I see what you're trying to do here. I like the idea of the input validation being "just another middleware" and I understand the limitations with the object-based API. Here's an alternative API that I quite like:

trpc.router({
  queries: {
    hello: trpc.resolver()
      .input(z.object({ test: z.string() }))
      .modifyContext(params => ({ ctx: ... })) 
      .resolve(params => ...)
  }
})

Basically you call trpc.resolver for each endpoint, and it provides you with a bunch of convenience methods for "appending" certain types of middlewares to the chain. It's easy to add additional methods (middleware types) to the Resolver object. You get auto-completion on the method names, so it's easy for users to discover all the different kinds of middleware logic they can implement. I personally really like the "pipeline" feel of it.

Optionally trpc.resolver() could accept a v9-style object-based resolver definition for users who prefer that style.

trpc.router({
  queries: {
    hello: trpc.resolver({
      input: z.object({ ... }),
      resolve(params){
        return { ... } 
      }
    })
  }
})

Honestly, there's no reason why I couldn't support this and map to the pipe-esque thing underneath, and I have considered this to make the migration path a bit easier for people already.

The reason why I've gone for the pipe-esque approach is that it's very hard currently to make complex middlewares reusable with the current model; middlewares that mutate an arbitrary context and return a new context. I know @anthonyshort found a solution for them but it's not super pleasant.

For a multi-tenant SaaS tool, for instance, this flow is quite common: you request to query/mutate a specific resource in which you typically want to send an organizationId on every request, then:

  1. Check the input is valid (e.g. {orgId: string; postId: string }
  2. Check that the currently authenticated user is part of orgId (this middleware needs a validate input that contains { orgId }
  3. Run resolver

This behavior is currently not possible in a nice way. In the new model, I can create a reusable middleware in each, make a generic resolver helper function that I could use like:

trpc.router({
  queries: {
    mySecretResource: trpc.resolver(
       canReadPost(z.object({ postId: z.string(), orgId: z.string()})),
       () => { /*  no further auth checks needed, yay */  }
     )
  }
})

Piping makes it easier to do complex mutations of the context object on the fly, and chainable ones that do it. For instance, two separate middlewares that do different things with the input (one validates and the other does auth check).

Might be ways of achieving the same things with the model you're suggesting. I'm not strongly opposed to this. Middlewares, in their current form, aren't super reusable as each middleware overwrites the previous unless you do some quite hairy magic (that probably uses pipe underneath).

I played around with these ideas in #15. Still not in love with the complexity. Open to ideas here.

Though there's an API that's even better. It avoids a bunch of repetitive calls to trpc.resolver() and doesn't represent such a dramatic shift from the v9 API.

trpc.router()
  .query("hello", route => route
    .input(z.object({ test: z.string() }))
    .modifyContext(params => ({ ctx: ... })) 
    .resolve(params => ...))
  .query("helloAgain", route => route
    .input(z.object({ test: z.string() }))
    .modifyContext(params => ({ ...params.context, newKey: "hey yo" })) 
    .resolve(params => ...))

Basically this closure API is a "power user feature" for users who prefer to write pipeline-style resolvers. It's easy to continue supporting the v9 API too. I think this is pretty close to optimal.

The chainable router doesn't scale enough for power users (like @mgreenw), but I'm still to do "proper" analytics on the results, I admit the proof and metrics are lacking here.

The client API 🤔

I'm on record as disliking the current tuple-based API.

The tuple-based API is there because we build on react-query which uses this API, I've done a shot on removing this already with a good alternative, see https://github.com/trpc/trpc/pull/1058, but decided against it as I didn't want to diverge from the API and maintain an alternative shadow API - there were some good thoughts here by @mmkal https://github.com/trpc/trpc/pull/1060#pullrequestreview-770148994

I think this proposed API is better but still doesn't feel RPC-like to me.

const greeting = await client.query(queries['post.add'], { hello: 'world' });

I think making the API worse in order to support Cmd-Click/"Go to definition" is a really bad tradeoff. If you put this to a vote on Twitter I'm guessing a vast majority of users would prefer a cleaner-looking client API to having CMD+click support. (I'm willing to be proven wrong here, but I'm very confident in this.)

I don't like the queries['post.add'](..) either, but this can simply be solved by having camelCase-convention rather than .-notation.

Massive GraphQL APIs are using camelCase as the only way of doing root queries, I don't see why tRPC couldn't simply be camel-cased rather than the strong distinction of sub-routers that you are suggesting.

I did a subtle change of using camel case in #21 which made all the calls instantly feel more pleasant.

The CMD+click support is lovely for keeping "in the flow" when coding, but I'm still not convinced myself if the trade-offs are worth it.

As for my ideal client API, I think it should be this:

const greeting = await trpc.query.post.add({ hello: 'world' });

I know you like this and I can see why, but it's arguably worse once you get over the hurdle. When you call a tRPC query now you get an instant fuzzy search on all of your app's queries and we don't need for a proxy - I think it's closer to RPC as-is.

This is an RPC library. Any API that doesn't feel like you're "calling" the remote function is bad, bordering on unacceptable. This is roughly how the API looked in v0.x and I would be super freakin happy to see it come back. If you don't do this, I suspect an alternative to tRPC will come along with this API and end up winning.

Maybe an alternative will come along and be better, that's fine by me, and if that day comes I'll join forces with them and help all tRPC users to have a solid migration path path. ​

I submitted a POC of this API here (types only, no runtime implementation): #22

No worries about runtime implementation, this playground has none! Thanks for doing this.


Further questions for you, @colinhacks:

KATT commented 2 years ago

Please break out your thoughts into the new issues @colinhacks