Fix FastifyInstance::ready type definition #802
Conversation
jeromemacias
force-pushed
the
ts_def_up
branch
from
f3033ca
to
c8a97fa
Compare
There was a problem hiding this comment.
ready could take up to three parameters.
How can we show that in typescript?
https://github.com/mcollina/avvio#ready
|
@delvedor done ;) |
test/types/index.ts
Outdated
| if (err) throw err | ||
| server.log.debug(context) | ||
| done() | ||
| }) |
fastify.d.ts
Outdated
| ready(readyListener?: () => void): void | ||
| ready(readyListener?: (err: Error) => void): void | ||
| ready(readyListener?: (err: Error, done: Function) => void): void | ||
| ready(readyListener?: (err: Error, context: FastifyInstance<HttpServer, HttpRequest, HttpResponse>, done: Function) => void): void |
There was a problem hiding this comment.
If called without the readyListener parameter, a Promise that resolves to the context is returned.
fastify.d.ts
Outdated
| ready(): Promise<FastifyInstance<HttpServer, HttpRequest, HttpResponse>> | ||
| ready(readyListener?: (err: Error) => void): void | ||
| ready(readyListener?: (err: Error, done: Function) => void): void | ||
| ready(readyListener?: (err: Error, context: FastifyInstance<HttpServer, HttpRequest, HttpResponse>, done: Function) => void): void |
There was a problem hiding this comment.
readyListener is not optional for the callback cases so the ? shouldn't be needed.
There was a problem hiding this comment.
You're right, good catch!
…ce::ready() ts definition
test/types/index.ts
Outdated
| }) | ||
|
|
||
| server.ready(function (err, done) { | ||
| if (err) throw err |
There was a problem hiding this comment.
Should this if statement and throw be removed if using the callback?
There was a problem hiding this comment.
Maybe we should call done(err) before throwing error, @mcollina any opinion ?
There was a problem hiding this comment.
There are not suggested paths on how handle this error, it's a decision that belongs to the user.
For the sake of the test, throw error is ok IMHO.
There was a problem hiding this comment.
if there is a done callback, the error should be passed there.
test/types/index.ts
Outdated
| }) | ||
|
|
||
| server.ready(function (err, context, done) { | ||
| if (err) throw err |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Checklist
npm run testandnpm run benchmark