KevinEdry / nestjs-trpc

Providing native support for decorators and implement an opinionated approach that aligns with NestJS conventions.
https://NestJS-tRPC.io
MIT License
82 stars 8 forks source link

ErrorShape is not applied #36

Closed johnny-y-wang closed 1 month ago

johnny-y-wang commented 1 month ago

It seems like errorShape as defined in this line is a no-op?

Per tRPC docs, the error formatter argument should be called errorFormatter, not errorShape. I might be missing something obvious here, but the disabled type-checking on this method does not inspire confidence :)

KevinEdry commented 1 month ago

Thats a good catch, its a trivial fix if you want to open a PR for it, if not I don't mind to do it instead. I disabled typescript on it because I am implementing a dynamic router with no actual types (since the router is built in run-time from your nestjs providers) which causes a typescript mismatch here. I might want to add a typeguard instead here to have some type-safety.

hrmcdonald commented 1 month ago

I noticed this as well and planned on opening a ticket. I have tested this locally and errorShape is just the wrong property. After renaming it to errorFormatter in the code it works as intended.


This might be worth a separate issue, but I'd actually much prefer another option altogether to enable implementing an onError handler that allows us to provide a service so that we can hook into dependency injection to log errors for example. Essentially exactly how context works today with TRPCContext services. The same setup asking us to provide something like a TRPCErrorHandler would be great.

This would just need to be provided like the context in the drivers:

KevinEdry commented 1 month ago

I noticed this as well and planned on opening a ticket. I have tested this locally and errorShape is just the wrong property. After renaming it to errorFormatter in the code it works as intended.

This might be worth a separate issue, but I'd actually much prefer another option altogether to enable implementing an onError handler that allows us to provide a service so that we can hook into dependency injection to log errors for example. Essentially exactly how context works today with TRPCContext services. The same setup asking us to provide something like a TRPCErrorHandler would be great.

This would just need to be provided like the context in the drivers:

Thats defiantly doable, feel free to create a new issue for it so I can label it correctly, you are more than welcome to implement it yourself (I could always use some help 😅).

johnny-y-wang commented 1 month ago

@KevinEdry Quick PR on your way to fix this. Once published, could you also bump npm version so I can update my local repo?

@hrmcdonald I agree. That'd be nice!

And, in case I haven't mentioned, big thank you to @KevinEdry for this awesome repo! Wanted to use tRPC and NestJS together for years now and this is finally possible :D

KevinEdry commented 1 month ago

@KevinEdry Quick PR on your way to fix this. Once published, could you also bump npm version so I can update my local repo?

@hrmcdonald I agree. That'd be nice!

And, in case I haven't mentioned, big thank you to @KevinEdry for this awesome repo! Wanted to use tRPC and NestJS together for years now and this is finally possible :D

Yup, once it is merged i'll bump the version. Thanks for the kind words ❤️

KevinEdry commented 1 month ago

Released in 🔗 https://github.com/KevinEdry/nestjs-trpc/releases/tag/1.6.1 🔗 https://www.npmjs.com/package/nestjs-trpc