chore: rework how TRPCError is created and add appropriate unit tests for it
#3880
Conversation
kamilogorek
requested review from
KATT,
trpc-bot,
sachinraja,
bautistaaa and
juliusmarminge
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@kamilogorek is attempting to deploy a commit to the trpc Team on Vercel. A member of the Team first needs to authorize it. |
KATT
changed the title
TRPCError is created and add appropriate unit tests for it
| expect(trpcError).toBeInstanceOf(TRPCError); | ||
| expect(trpcError).toBeInstanceOf(Error); | ||
| }); | ||
| test('should extend original Error class', () => { |
There was a problem hiding this comment.
There is more than 1 top-level thing tested in this file, that's why it was split using describe blocks :)
Head branch was pushed to by a user without write access
| opts.message ?? getMessageFromUnknownError(opts.cause, code); | ||
| const cause: Error | undefined = | ||
| opts.message ?? getMessageFromUnknownError(opts.cause, opts.code); | ||
| const cause = | ||
| opts.cause !== undefined ? getErrorFromUnknown(opts.cause) : undefined; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore https://github.com/tc39/proposal-error-cause | ||
| super(message, { cause }); |
There was a problem hiding this comment.
I don't have the history on this, what's the reason for not following the Error constructor?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error#syntax
There was a problem hiding this comment.
The Error constructor accepts a message as the first argument, and then options (or filename and lineNumber), whereas TRPCError only accepts an object combining both the message and options. IMO it would be better to follow the same constructor and extend the options to add the code, but I'm sure there's a reason why it was made like this (but that would require a breaking change).
There was a problem hiding this comment.
Ah ok, I don't know the history here as well. From what I see it's most likely because message is optional and not always known at compile time. It could feel awkward to do new TRPCError(null, { code: 420 });. Just a guess.
There was a problem hiding this comment.
If there is more that can be done that would result in something breaking we can do it on the next branch
🎯 Changes
Resolves one outstanding note from the codebase (#331 (comment)).
As for why it didn't work previously, it was most likely due to the odd behavior of
in some environments. This is however redundant to call in non-ES6 environments, which based on #3393 is not the case for tRPC, as it requires Proxy API, which is ES6+ only.
We've also been there, but we're tied to pre-ES6 envs - https://github.com/getsentry/sentry-javascript/blob/33243242effa68390d16203917475ce26c8f2037/packages/utils/src/error.ts#L14-L17
✅ Checklist