-
Notifications
You must be signed in to change notification settings - Fork 101
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
feat: add typings #90
Conversation
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.
Good work, a couple of nits.
@@ -0,0 +1,35 @@ | |||
import fastify = require("fastify"); |
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 think those come witha license header that we should likely maintain here with some notes.
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 modified the typings (last commit) to use fastify.Plugin and did a minor change on the setHeaders function. Since it is not direct copy of the typings from definitelyTyped I should we still include the header?
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.
Maybe I could add this?
// Original Definitions by: Leonhard Melzer <https://github.com/leomelzer>
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 creator is a colleague of mine, I'll consult with him.
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.
We could follow the DefinitelyTyped proposal: https://github.com/DefinitelyTyped/DefinitelyTyped#edit-an-existing-package
// Definitions by: Jannik <https://github.com/jannikkeye>
// Leo <https://github.com/leomelzer>
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.
That sounds good. I'll set it like you proposed and omit all the DefinitelyTyped related stuff.
test/types/index.ts
Outdated
|
||
const app = Fastify() | ||
|
||
app.register<fastifyStatic.FastifyStaticOptions>(fastifyStatic, options) |
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 add a test for sendFile as well?
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.
Added a test case.
Thanks @jannikkeye 🎉 We should make sure to remove the original typings from DefinitetlyTyped to remove confusion:
In DefinitelyTyped: => 2.2.0 or whatever version this release will become :) Can you take care of that after the release? |
@leomelzer yes I can take care of that. I'll have to do it for another package as well anyway. 👍 |
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
This PR adds the typings from DefinitelyTyped (slightly modified) to the main org repo.