fastify-basic-auth icon indicating copy to clipboard operation
fastify-basic-auth copied to clipboard

Fix validate() return type

Open flying-sheep opened this issue 3 years ago • 15 comments

Checklist

flying-sheep avatar Sep 15 '21 14:09 flying-sheep

all done. I also completely split the possible function signatures.

flying-sheep avatar Sep 16 '21 09:09 flying-sheep

Why did the tests not run?

flying-sheep avatar Nov 03 '21 09:11 flying-sheep

They did.

There are errors:

  index.test-d.ts:16:4
  ✖  15:44  Parameter username implicitly has an any type.                                                                                               
  ✖  15:54  Parameter password implicitly has an any type.                                                                                               
  ✖  15:64  Parameter req implicitly has an any type.                                                                                                    
  ✖  15:69  Parameter reply implicitly has an any type.                                                                                                  
  ✖  16:4   Parameter type string is not identical to argument type any.                                                                                 
  ✖  17:4   Parameter type string is not identical to argument type any.                                                                                 
  ✖  18:4   Parameter type FastifyRequest<RouteGenericInterface, Server, IncomingMessage> is not identical to argument type any.                         
  ✖  19:4   Parameter type FastifyReply<Server, IncomingMessage, ServerResponse, RouteGenericInterface, unknown> is not identical to argument type any.  

  8 errors

mcollina avatar Nov 03 '21 09:11 mcollina

Ah, I guess it’s this one: https://github.com/microsoft/TypeScript/issues/598

flying-sheep avatar Nov 03 '21 10:11 flying-sheep

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.

stale[bot] avatar Apr 16 '22 09:04 stale[bot]

So how do we fix this? I can’t write a test if TypeScript bugs out on me

flying-sheep avatar Apr 19 '22 08:04 flying-sheep

cc @fastify/typescript can you help?

mcollina avatar Apr 19 '22 09:04 mcollina

@flying-sheep you need to address the type checker error, e.g change validatePromise (username, password, req, reply) to validatePromise (username: string, password: string, req: FastifyRequest, reply: FastifyReply)

evanshortiss avatar Apr 19 '22 18:04 evanshortiss

Hmm, I thought the whole point of the test was to check if the types get inferred properly …

flying-sheep avatar Apr 20 '22 09:04 flying-sheep

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 done, but had some issues getting that to work.

evanshortiss avatar Apr 20 '22 15:04 evanshortiss

Welp, maybe not possible then. Too bad.

flying-sheep avatar Apr 21 '22 09:04 flying-sheep

Yeah, seems like it's necessary to explicitly define the type for the Promise-based approach. Exporting those FastifyBasicAuthValidateFnPromise and FastifyBasicAuthValidateFnCallback seems like an alright compromise perhaps?

At least it will provide a hint via intellisense like so:

Screenshot 2022-04-21 at 1 19 46 PM

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'
})

evanshortiss avatar Apr 21 '22 20:04 evanshortiss

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 FastifyBasicAuthValidateFnPromise. As it stands, users have to manually say that, which would be a regression.

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">);
  ...
}

flying-sheep avatar Apr 22 '22 09:04 flying-sheep

I looked into this and I am not sure what is going on. The main issue might be on the FastifyPlugin generic side. I'll look deeper into this ASAP. Thanks for the PR.

fox1t avatar Apr 22 '22 13:04 fox1t

This isn't specific to FastifyPlugin AFAICT. Just a quirk of TS. Take a look at this playground example that exhibits the same behaviour. A TypeScript wizard can possibly figure out some clever trick though!

evanshortiss avatar Apr 22 '22 18:04 evanshortiss