types: allow any return value for error#222
types: allow any return value for error#222sirlancelot wants to merge 2 commits intofastify:mainfrom
Conversation
Signed-off-by: Matthew Pietz <sirlancelot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fdawgs
changed the title
Fdawgs
left a comment
Fdawgs
left a comment
There was a problem hiding this comment.
Thanks @sirlancelot, could you please add an associated test?
| keys: Set<string> | string[]; | ||
| auth?: (key: string, req: FastifyRequest) => boolean | Promise<boolean> | undefined; | ||
| errorResponse?: (err: Error) => { error: string }; | ||
| errorResponse?: (err: Error) => unknown; |
There was a problem hiding this comment.
Sorry for requesting changes after approving.
Could you add more test cases in addition to the existing string-based fixture using err.message? Since we now support returning unknown instead of string, it would be helpful to cover those cases as well.
For example:
https://github.com/sirlancelot/fastify-bearer-auth/blob/patch-1/types/index.test-d.ts#L6-L13
const pluginOptions: FastifyBearerAuthOptions = {
keys: new Set(['foo']),
auth: (_key: string, _req: FastifyRequest) => { return true },
errorResponse: (err: Error) => { return { error: err.message } },
contentType: '',
bearerType: '',
addHook: false
}There was a problem hiding this comment.
I'm not exactly sure what sort of testing is possible for unknown types. I think that's the point of unknown. All errors in JavaScript start out as unknown or any, depending on configuration, it's up to the implementation to determine what type it actually is.
There was a problem hiding this comment.
Thanks for your reply.
How about adding a test case using primitive types?
This would ensure that the return value accepts types other than just string.
Checklist
npm run test && npm run benchmark --if-presentand the Code of conduct
Description
The return value of the
errorResponseconfig option function gets passed directly toreply.sendand as such can support a wide range of possible values. Since the documentation encourages using this option to supply one's own error shape, it shouldn't make assumptions about the type, so I've set it tounknownin order to fix the type error I get when supplying my own error shape.