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

[RFC] Error callback and no more `return { data: X }` #17

Closed KATT closed 2 years ago

KATT commented 2 years ago

Instead of having all procedures to return a { data: X } | { error: Y } shape, use an error({ code: 'X ', /* additional data */})

Questions

Benefits

Example use

export const appRouter = trpc.router({
  queries: {
    'post.byId': trpc.resolver(
      trpc.zod(
        z.object({
          id: z.string(),
        }),
      ),
      ({ input }) => {
        const post = postsDb.find((post) => post.id === input.id);
        if (!post) {
          return trpc.error({ code: 'NOT_FOUND' });
        }
        return post;
      },
    ),
});

The result of the above is:

type Result =
  | Post
  | {
      error: {
        code: 'BAD_REQUEST';
        zod: z.ZodFormattedError<{
          id: string;
        }>;
      };
    }
  | {
      error: {
        code: 'NOT_FOUND';
      };
    };

Details


Suggestion by @mmkal

danielyogel commented 2 years ago

@KATT @mmkal

"Instead of having all procedures to return a { data: X } | { error: Y } shape, use an error({ code: 'X ', / additional data /})"

I believe this change is indeed better than the first (error/data) version - less verbose and more familiar for both people who are regular to throw errors, as well for people who are regular to functional error handling (like Either type).

The error fn adds a Symbol that is easily identifiable both in code and in generics

Did you consider using an Either type, or even just importing an existing tree shakeable one like fp-ts's Either? From what I understand, you will get the following:

pros:

  1. Safety in the type level & runtime code - without the need to use Symbol, internal code like ExcludeErrorLike OnlyErrorLike or any new mechanism.
  2. Interoperability with the whole Monads, Functors and similar type classes, and with any TypeScript library that use this kind of code (Zod?).
  3. Very familiar to functional programmers (Haskell, Scala, Rust, Functional TS).
  4. Battle tested code.

    cons:

  5. Having to wrap the 'success' branch with a "trpc.success(results)" (named 'Right' in the 'Either' type) function as the 'error' branch.
  6. When returning undefined, which is common for mutations, you'll have to return "trpc.success(undefined)". actually I think this is solvable on the type level, returning Either | undefined | null ...
KATT commented 2 years ago

I believe this change is indeed better than the first (error/data) version - less verbose and more familiar for both people who are regular to throw errors, as well for people who are regular to functional error handling (like Either type).

I'm pretty convinced on it too!

The error fn adds a Symbol that is easily identifiable both in code and in generics

Did you consider using an Either type, or even just importing an existing tree shakeable one like fp-ts's Either? From what I understand, you will get the following:

Either is pretty identical to my approach apart from the approach I'm suggesting makes it impossible for a user to accidentally return something that is interpreted as an error.

With fp-ts' approach one could return a { _tag: 'Left', left: T } and it'd be interpreted as an error.

https://github.com/gcanti/fp-ts/blob/6d6f7d8b725619db509199742b1079d9e7922dc5/src/Either.ts#L63-L85

That said, maybe the Symbol is complicating things unnecessarily? It is a bit of an extreme edge case to cover.

pros:

  1. Safety in the type level & runtime code - without the need to use Symbol, internal code like ExcludeErrorLike OnlyErrorLike or any new mechanism.

Both solutions have similar underlying mechanisms.

  1. Interoperability with the whole Monads, Functors and similar type classes, and with any TypeScript library that use this kind of code (Zod?).
  2. Very familiar to functional programmers (Haskell, Scala, Rust, Functional TS).

This is interesting.

  1. Battle tested code.

This part of the code is like a grain of sand in a desert. I am pretty confident I'll be able to test this appropiately.

cons:

  1. Having to wrap the 'success' branch with a "trpc.success(results)" (named 'Right' in the 'Either' type) function as the 'error' branch.

Yeah, I don't want this. I'd rather letting the compiler automatically add Right<T> on it (or Left<T>, I always mix them up)

  1. When returning undefined, which is common for mutations, you'll have to return "trpc.success(undefined)". actually I think this is solvable on the type level, returning Either | undefined | null ...

I think it's solvable too.

danielyogel commented 2 years ago

Either is pretty identical to my approach apart from the approach I'm suggesting makes it impossible for a user to accidentally return something that is interpreted as an error. With fp-ts' approach one could return a { _tag: 'Left', left: T } and it'd be interpreted as an error.

I'm not 100% sure this is true, because Either<R,E> gets two type variables, which may used to enforce the return types of both branches. I will test that when have time.

Yeah, I don't want this. I'd rather letting the compiler automatically add Right on it (or Left, I always mix them up)

Just to clarify, you already use a "left" like helper (the trpc.error() utility), the only difference in Either is that you will also have to use something like "trpc.success()".

Screen Shot 2022-01-24 at 11 51 43
  1. Beside that, I've just played with the playground, as pictured above, and tried to use the return type in the client in a type safe way - so far failed to do that. Maybe it's just my fault, let me know if there's a way. This is Something "Either" has built in more than one utility function (isLeft / isRight / map / bind), and I believe this (type-safety) is a must-have feature of any acceptable solution.

  2. Another thing that just crossed my mind, is that maybe implementing this stuff in the tRPC level is not a must. A user can just return left/right, instead of throwing, in the current version of tRPC, white treating any non 200 code as unexpected error/bug. TS's type inference will do the work.

mmkal commented 2 years ago

Either is pretty identical to my approach apart from the approach I'm suggesting makes it impossible for a user to accidentally return something that is interpreted as an error. With fp-ts' approach one could return a { _tag: 'Left', left: T } and it'd be interpreted as an error.

I'm not 100% sure this is true, because Either<R,E> gets two type variables, which may used to enforce the return types of both branches. I will test that when have time.

If I were building an fp-ts tutorial site and I had a query getSampleEitherValue which return { _tag: 'Left', left: 123 }, it would confuse trpc into thinking the procedure had failed at runtime. At compile time it might get tripped up too.

Yeah, I don't want this. I'd rather letting the compiler automatically add Right on it (or Left, I always mix them up)

Just to clarify, you already use a "left" like helper (the trpc.error() utility), the only difference in Either is that you will also have to use something like "trpc.success()".

I do think that might be a bit of an adoption concern. I wonder if there's a good way to canvas opinion on this (although things like twitter polls might just find trpc acolytes who are more happy than the average potential user who'd be more likely to be put off by needing to put trpc.something all over the place.

Screen Shot 2022-01-24 at 11 51 43
  1. Beside that, I've just played with the playground, as pictured above, and tried to use the return type in the client in a type safe way - so far failed to do that. Maybe it's just my fault, let me know if there's a way. This is Something "Either" has built in more than one utility function (isLeft / isRight / map / bind), and I believe this (type-safety) is a must-have feature of any acceptable solution.
  2. Another thing that just crossed my mind, is that maybe implementing this stuff in the tRPC level is not a must. A user can just return left/right, instead of throwing, in the current version of tRPC, white treating any non 200 code as unexpected error/bug. TS's type inference will do the work.
  • related to (1), if ('data' in greeting){ console.log(greeting.data.greeting) } works.
  1. If you type the "leftish" type as { error: ..., data?: undefined } then you can do console.log(greeting.data?.greeting)
  2. Yeah tbh on my previous team we put a ton of work into typing errors, but it didn't work out that well. We still had to handle "unknown" errors anyway, and the juice wasn't really worth the squeeze. On my current team we don't do that and haven't really missed it - and we move faster. Again curious if @KATT you have any sense of how many people want help from trpc with error responses?
KATT commented 2 years ago
  1. Yeah tbh on my previous team we put a ton of work into typing errors, but it didn't work out that well. We still had to handle "unknown" errors anyway, and the juice wasn't really worth the squeeze. On my current team we don't do that and haven't really missed it - and we move faster. Again curious if @KATT you have any sense of how many people want help from trpc with error responses?

Not really anyone asking for it. It started as a challenge to myself but I can see it being quite useful in some cases.... although still not sure it'll make it in the release.

Will spin-off that discussion to #28 and merge this one for now