Skip to content
This repository has been archived by the owner on Dec 19, 2022. It is now read-only.

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

Merged
merged 5 commits into from
Feb 1, 2022

Conversation

KATT
Copy link
Member

@KATT KATT commented Jan 23, 2022

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

Questions

  • Should it really be called error() - maybe an invalid()/bad()-helper would be more suitable
    • It's not an Error, it's more about signalling a bad request - proper thrown errors will be handled in a similar fashion of what they are today.

Benefits

  • Less typing for happy paths
  • More explicit error handling
  • When returning undefined, which is common for mutations, you don't have to return anything in the fn

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

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

Suggestion by @mmkal

@danielyogel
Copy link
Collaborator

danielyogel commented Jan 23, 2022

@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:

  1. Having to wrap the 'success' branch with a "trpc.success(results)" (named 'Right' in the 'Either' type) function as the 'error' branch.
  2. 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
Copy link
Member Author

KATT commented Jan 24, 2022

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.

@KATT KATT added the ❕ RFC Request for comments - please comment! label Jan 24, 2022
@danielyogel
Copy link
Collaborator

danielyogel commented Jan 24, 2022

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.

  • related to (1), if ('data' in greeting){ console.log(greeting.data.greeting) } works.

@mmkal
Copy link
Contributor

mmkal commented Jan 25, 2022

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
Copy link
Member Author

KATT commented Feb 1, 2022

  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

@KATT KATT marked this pull request as ready for review February 1, 2022 20:42
@KATT KATT merged commit dce2c79 into main Feb 1, 2022
@KATT KATT deleted the katt/response-error-marker branch February 1, 2022 20:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
❕ RFC Request for comments - please comment!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants