-
-
Notifications
You must be signed in to change notification settings - Fork 28
Fix validate() return type #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
mcollina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test to https://github.com/fastify/fastify-basic-auth/blob/master/index.test-d.ts?
Co-authored-by: KaKa <[email protected]>
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this throw instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The document is using return and it is actually allowed by this line.
Line 24 in fdfe2fb
| result.then(done, done) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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