feat(typescript): add typescript definitions #19
Conversation
|
Sure guys! Will add some unit tests |
|
Added some types test, I've used |
package.json
Outdated
| @@ -23,6 +23,9 @@ | |||
| }, | |||
| "homepage": "https://github.com/fastify/fastify-sensible#readme", | |||
| "devDependencies": { | |||
| "@types/http-errors": "^1.6.1", | |||
| "@types/proxy-addr": "^2.0.0", | |||
| "@types/vary": "^1.1.0", | |||
There was a problem hiding this comment.
It seems these are used in the types. These should be dependencies, but I’m -1 for that. Would you mind just integrating those types with ours?
There was a problem hiding this comment.
Hi @mcollina , I don't mind doing that. Another alternative is to upload fastify-sensible types definition on @types domain (https://github.com/DefinitelyTyped/DefinitelyTyped). What do you think about this option?
There was a problem hiding this comment.
I'm -1 to that, types on @types tend to be of lower quality, harder to maintain, and essentially more problematic than the ones we maintain directly.
There was a problem hiding this comment.
@mcollina , there is a caveat to that... @types/proxy-addr and @types/vary depend on @types/node, we use in import * as http from 'http'. The @types/node has a lot of definitions that are used by http.d.ts, thus the need to import everyone. Is this a problem? If so, we can either leave @types/node as dependency or simply @types/proxy-addr and @types/vary.
There was a problem hiding this comment.
@types/node is fine, all ts users will add that.
There was a problem hiding this comment.
@mcollina why not just peerDependencies the other packages?
package.json
Outdated
| @@ -28,6 +28,9 @@ | |||
| "tap": "^12.1.1" | |||
| }, | |||
| "dependencies": { | |||
| "@types/http-errors": "^1.6.1", | |||
| "@types/proxy-addr": "^2.0.0", | |||
| "@types/vary": "^1.1.0", | |||
There was a problem hiding this comment.
@mcollina , just to clarify, the requested changes were regarding using @types as dependencies, instead of devDependencies, or importing those types definitions (http-errors.d.ts, proxy-addr.d.ts, vary.d.ts and its other dependencies) inside the project itself?
There was a problem hiding this comment.
I'm exactly -1 to this approach. We should not do this.
There was a problem hiding this comment.
@hspedro we'd like to keep types as simple as possible with as minimal dependencies as possible too. The solution here is to implement any relevant parts of @types/http-errors in the type files so we do not need to rely on the package as a dependency. If things change in the future we can update the types in this repo and ship without having to deal with any external things.
For example you're using this type definition from @types/http-errors:
interface HttpError extends Error {
status: number;
statusCode: number;
expose: boolean;
headers?: {
[key: string]: string;
};
[key: string]: any;
}This is a very simple interface that could be copied in your type file.
I hope this clarifies some things!
There was a problem hiding this comment.
got it @Ethan-Arrowood. Thanks, will update with requested changes as soon as possible.
There was a problem hiding this comment.
@Ethan-Arrowood , I've managed to refactor the dependencies for the vast majority. But, how about the more complex types such as HttpServer that comes from @types/node, can I leave @types/node as a project dep or should I reimplement the http.Server, http.IncomingMessage, http.ServerResponse?
There was a problem hiding this comment.
Leave @types/node as a dependency. If users are using Fastify with TypeScript they will have it installed so it is okay for us to include.
hspedro
force-pushed
the
ts-definitions
branch
2 times, most recently
from
361a430 to
d5a6081
Compare
package.json
Outdated
| @@ -23,11 +23,12 @@ | |||
| }, | |||
| "homepage": "https://github.com/fastify/fastify-sensible#readme", | |||
| "devDependencies": { | |||
| "fastify": "^2.0.0-rc.2", | |||
| "fastify": "^2.0.0", | |||
There was a problem hiding this comment.
update fastify to latest v2 release
There was a problem hiding this comment.
Hi @Ethan-Arrowood , can you confirm request regarding this change? Thanks
There was a problem hiding this comment.
Actually keep it as is. I believe the ^2.0.0 resolves to latest of version 2 anyways.
tsconfig.json
Outdated
| "module": "commonjs", | ||
| "noEmit": true, | ||
| "strict": true, | ||
| "noImplicitAny": false, |
There was a problem hiding this comment.
I'd like this to be true. What types are any when this is enabled?
There was a problem hiding this comment.
Actually, this should be true. No types should default to any
The default export is the fastifySensible plugin, whereas the http errors are also being exported but namely.
The tests were based on fastify-jwt and other projects that have type definition and types test. The test will cover types on each of the modules: 'assert', 'errorHandler', 'forwarded', 'httpErrors', 'is', 'proxyaddr', 'to' and 'vary' (coverage was based on current JS tests). As only the types will be checked, `tsc` is enough for that.
Added `tsconfig.json` to be used by `tsc` when package.json 'test' script.
The default export is the fastifySensible plugin, whereas the http
errors are also being namely exported on index.d.ts.
The need came from a project using TypeScript, in which we would
like to augment fastify plugin using fastify-sensible's httpErrors. Thus,
needing the definitions.