Conversation
mcollina
left a comment
There was a problem hiding this comment.
Could you add a test to https://github.com/fastify/fastify-basic-auth/blob/master/index.test-d.ts?
|
all done. I also completely split the possible function signatures. |
| expectType<string>(password) | ||
| expectType<FastifyRequest>(req) | ||
| expectType<FastifyReply>(reply) | ||
| if (Math.random() > 0.5) return new Error() |
There was a problem hiding this comment.
The document is using return and it is actually allowed by this line.
Line 24 in fdfe2fb
There was a problem hiding this comment.
I would prefer two asserts group instead:
- One returning
Promise<Error> - One returning
Promise<void>
|
Why did the tests not run? |
|
They did. There are errors: |
|
Ah, I guess it’s this one: microsoft/TypeScript#598 |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
So how do we fix this? I can’t write a test if TypeScript bugs out on me |
|
cc @fastify/typescript can you help? |
|
@flying-sheep you need to address the type checker error, e.g change |
|
Hmm, I thought the whole point of the test was to check if the types get inferred properly … |
|
Wow, sorry, you're right. Looks like an odd TS behaviour. Not sure what's best here. Perhaps export two definitions? export type FastifyBasicAuthValidateFnCallback = (username: string, password: string, req: FastifyRequest, reply: FastifyReply, done: (err?: Error) => void) => void
export type FastifyBasicAuthValidateFnPromise = (username: string, password: string, req: FastifyRequest, reply: FastifyReply) => Promise<Error|void>
export interface FastifyBasicAuthOptions {
validate: FastifyBasicAuthValidateFnPromise|FastifyBasicAuthValidateFnCallback;
authenticate?: boolean | { realm: string };
header?: string;
}This way you can do something like this? app.register(fastifyBasicAuth, {
validate: function (username, password, req, reply) {
return new Promise((resolve, reject) => resolve())
} as FastifyBasicAuthValidateFnPromise
})Maybe there's a better solution? I quickly tried conditional types (to return void or a Promise) based on the presence of |
|
Welp, maybe not possible then. Too bad. |
|
Yeah, seems like it's necessary to explicitly define the type for the Promise-based approach. Exporting those At least it will provide a hint via intellisense like so: The test case could become: app.register(fastifyBasicAuth, {
validate: function validateCallback (username, password, req, reply) {
expectType<string>(username)
expectType<string>(password)
expectType<FastifyRequest>(req)
expectType<FastifyReply>(reply)
return new Promise<void>((resolve, reject) => {resolve()})
} as FastifyBasicAuthValidateFnPromise,
header: 'x-forwarded-authorization'
}) |
|
Maybe we can get a TypeScript wiz in here to help. It would be great to get it to a state where it’s correct but TypeScript can also infer I tried this, but it didn’t change anything: type IsArg5Valid<T extends (...a: any) => any, E> = Parameters<T>['length'] extends 5 ? {} : E;
export interface FastifyBasicAuthOptions {
validate:
FastifyBasicAuthValidateFnPromise
| (FastifyBasicAuthValidateFnCallback & IsArg5Valid<FastifyBasicAuthValidateFnCallback, "You need to specify a “done” callback">);
...
} |
|
I looked into this and I am not sure what is going on. The main issue might be on the |
|
This isn't specific to |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the TypeScript definitions to properly support both callback-based and promise-based validation patterns for the validate function. The change splits the previous single function signature into a union type that clearly distinguishes between the async/promise pattern (which can return Error | void) and the callback pattern (which receives a done callback).
- Changed
validatefrom a single overloaded signature to an explicit union of two function types - Added test coverage for returning an
Errorfrom an async validate function
Reviewed Changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| index.d.ts | Splits the validate function signature into a union type: one for promise-based validation (returning Promise<Error | void>) and one for callback-based validation (accepting a done parameter) |
| index.test-d.ts | Adds test case demonstrating that async validate functions can conditionally return an Error object |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Checklist
npm run testandnpm run benchmarkand the Code of conduct