-
-
Notifications
You must be signed in to change notification settings - Fork 34
Types: Add "pg" property to FastifyInstance #68
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
Conversation
As recommended in the guide for creating TypeScript compatible Fastify plugins, properties that the Fastify instances are decorated with should also in the types get added to those instances: https://www.fastify.io/docs/latest/TypeScript/#creating-a-typescript-fastify-plugin This change follows up on #63 by adding that.
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.
Can you please add a unit test?
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.
Nice catch!
LGTM after adding tests.
This became more complex. For some odd reason this module has an undocumented, yet tested and verified, capability that allows it to expose The latter is achieved by adding the module multiple times, each time adding a So: I changed course here a bit. Adding documentation for that property + tests that ensures that one always take one or the other approach. Also clarified the intention that one should manually be adding the Up next, in another PR, I would want to do a breaking change: Have That way it's actually possible to expose types for both out of the box, without any users having to manually add the correct one for them. How does that sound? Can we merge this PR as it is with the documentation and test improvements? And I'll open a new one that takes care of a possible breaking change. |
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.
lgtm
As recommended in the guide for creating TypeScript compatible Fastify plugins, properties that the Fastify instances are decorated with should also in the types get added to those instances: https://www.fastify.io/docs/latest/TypeScript/#creating-a-typescript-fastify-plugin
This change follows up on #63 by adding that.
Checklist
npm run test
andnpm run benchmark
documentation is changed or addedand the Code of conduct