Fix typescript typings #17
Conversation
There was a problem hiding this comment.
@mksony Thanks a lot for submitting this PR!
There are, however, a few issues I would like you to take a look at.
Also, while I like the tsd tool, the version you used is not compatible with node.js 8, which makes the tests fail (see the Checks tab). Please try to use an older version that would work so that this plugin can maintain its current compatibility level.
| } | ||
|
|
||
| export = FastifyCsrfPlugin; | ||
| declare const fastifyCsrf: fastify.Plugin< |
There was a problem hiding this comment.
The fastify.Plugin type accepts only the Options type parameter, see: https://github.com/fastify/fastify/blob/master/types/plugin.d.ts#L10
This also makes HttpServer, HttpRequest and HttpResponse obsolete, so they can be removed from this file completely.
There was a problem hiding this comment.
Yeah, I thought so too at first, but since we currently depend on fastify@2.x we need to look at https://github.com/fastify/fastify/blob/2.x/fastify.d.ts#L26 and there we the typings are less sophisticated unfortunately 馃槓
So should we keep it the way it is for now?
There was a problem hiding this comment.
Oh, my bad! Thanks for pointing this out 馃憤 .
Please keep it the way it is.
lib/fastifyCsrf.d.ts
Outdated
| key?: string; | ||
| value?: (req: fastify.FastifyRequest) => string; | ||
| cookie?: fastify.CookieSerializeOptions | boolean; | ||
| ignoreMethods?: Array<string>; |
There was a problem hiding this comment.
Please preserve the code style of the main code. string[] is preferable to to Array<string>, and you should use tabs for indentation instead of 2 spaces (I know, indentation is not completely consistent in the code base. Let's improve that a little bit :)).
Also, please use single quotes instead of double.
There was a problem hiding this comment.
Agreed 馃檪 See 2b82425.
package.json
Outdated
| @@ -5,7 +5,8 @@ | |||
| "main": "lib/fastifyCsrf.js", | |||
| "types": "lib/fastifyCsrf.d.ts", | |||
| "scripts": { | |||
| "test": "tap test/*.js --cov" | |||
| "test": "tap test/*.js --cov && npm run typescript", | |||
| "typescript": "tsd" | |||
There was a problem hiding this comment.
Unfortunately, the naming here seems a little misleading and confusing. The script itself seems superfluous since it does not configure tsd using env variables or arguments. Could you change the test script to tap test/*.js --cov && tsd, please?
There was a problem hiding this comment.
Yep you're right. I changed it to your suggestion. See 19cea1d
test/fastifyCsrf.test-d.ts
Outdated
| @@ -0,0 +1,26 @@ | |||
| import * as fastify from "fastify"; | |||
There was a problem hiding this comment.
Please update the code style as specified in comment above.
test/fastifyCsrf.test-d.ts
Outdated
| <form method = "post" action="/data"> | ||
| <input type="text" name"field_name"/> | ||
| <input type="hidden" value="${request.csrfToken()}" | ||
| name="_csrf /* this token can be sent in request header as well */"/> |
There was a problem hiding this comment.
I can see you have copy-pasted this from README :).
Unfortunately, JS comments (/* */) do not play nicely with HTML, could you, please, remove it?
There was a problem hiding this comment.
Yeah good, point 馃憤 See 188442c
You're welcome. I am happy to contribute 馃檪
I addressed all the inline comments and for #17 (comment) I would need feedback from you, please.
Thanks for pointing this out. The people maintaining Which option should we go for? |
There was a problem hiding this comment.
Thanks a lot for addressing all the issues!
I think it is safe to downgrade tsd for now, since it does not limit our use of it at the moment, is introduced in this PR and it is only a dev dependency.
There is also the option of dropping Node 8 support, but that ought to be up to the project owner when they feel like it, and preferably in a separate PR.
@Tarang11 I am approving this PR 馃殌 , IMO it looks good. Please do a final review if you feel like it and merge it when convenient.
| } | ||
|
|
||
| export = FastifyCsrfPlugin; | ||
| declare const fastifyCsrf: fastify.Plugin< |
There was a problem hiding this comment.
Oh, my bad! Thanks for pointing this out 馃憤 .
Please keep it the way it is.
Closes #15
I added two commits, one with the changes which made the typings work as described in the issue and another one which would add a minimal test and related setup.
Please let me know what you think about the included test, I can easily remove it if it should not be part of the regular tests 馃檪
It was inspired by e.g. https://github.com/fastify/fastify-cors