TypeScript Documentation #669
Conversation
delvedor
added
discussion
docs/TypeScript.md
Outdated
| @@ -2,10 +2,17 @@ | |||
|
|
|||
| <a id="typescript"></a> | |||
| ## TypeScript | |||
| Fastify is shipped with the a typings file so it should "just work" when used in a TypeScript application. | |||
| Fastify is shipped with the a typings file so it should "just work" when used in a TypeScript application.<br/> | |||
| *You should also install `@types/node` and `@types/pino`.* | |||
There was a problem hiding this comment.
can you make it explicit
npm i @types/node @types/pino
There was a problem hiding this comment.
@types/node isn't always necessary. Typescript projects only need it for accessing node built-in classes. Most projects do need the built-in classes, so Typescript programmers are used to installing @types/node first thing as a matter of habit.
There was a problem hiding this comment.
It's not yet clear to me whether either @types/node or @types/pino is actually needed in devDependencies for Fastify. I'll look into that this weekend.
There was a problem hiding this comment.
Since Fastify ships with a particular version of pino, it would make sense for @types/pino to stay in dependencies so that the pino version and its typings will stay in sync in this repo.
There was a problem hiding this comment.
We try to ship always the latest.
I don't want to force not-typescript users to download useless stuff :)
There was a problem hiding this comment.
That makes sense @nwoltman. Spares TS devs from figuring out the right version of typings to use.
There was a problem hiding this comment.
@delvedor I don't want to force not-TypeScript users to download useless stuff either, but we're already doing that by including fastify.d.ts, which has a direct dependency on @types/pino.
TypeScript users should include @types/node themselves because they choose which version of Node to using, but since fastify depends on pino, it is fastify that chooses which version of pino to use.
We won't have to worry about this though if pino includes its own typings, which might happen soon at some point (see this discussion in the pino repo).
There was a problem hiding this comment.
I'd be careful with that word "soon". Unless someone steps up with at least a PR, there will not be type in the pino module any time soon.
docs/TypeScript.md
Outdated
| ## Types support | ||
| We do care about the TypeScript community, but the framework is written in plain JavaScript and currently no one of the core team is a TypeScript user while only one of the collaborators is. | ||
| We do our best to have the typing updated with the latest version of the API, but *it can happen* that the typings are not in sync.<br/> | ||
| Luckly this is Open Source and you can contribute to fix them, we will be very happy to accept the fix and release it as soon as possible as patch release. Checkout the [contributing](#contributing) rules! |
docs/TypeScript.md
Outdated
| Fastify is shipped with the a typings file so it should "just work" when used in a TypeScript application. | ||
| Fastify is shipped with the a typings file so it should "just work" when used in a TypeScript application.<br/> | ||
| *You should also install `@types/node` and `@types/pino`.*<br/> | ||
| `npm i @types/node @types/pino --save` |
There was a problem hiding this comment.
I assume this is to reduce bloat, but it might be confusing for folks using TypeScript since the versions aren't specified here whereas if included in package.json it would "just work" as claimed.
There was a problem hiding this comment.
How about we rephrase it as:
Fastify is shipped with the a typings file, but it still require dependent types:
npm i @types/node @types/pino@4 --save
We might still ship @types/pino as a dependency, I'm not 100% sold on that. I would actually prefer if pino shipped with its own typings file.
| ## Types support | ||
| We do care about the TypeScript community, but the framework is written in plain JavaScript and currently no one of the core team is a TypeScript user while only one of the collaborators is. | ||
| We do our best to have the typing updated with the latest version of the API, but *it can happen* that the typings are not in sync.<br/> | ||
| Luckly this is Open Source and you can contribute to fix them, we will be very happy to accept the fix and release it as soon as possible as a patch release. Checkout the [contributing](#contributing) rules! |
| @@ -64,6 +64,8 @@ | |||
| "node": ">=4.5" | |||
| }, | |||
| "devDependencies": { | |||
| "@types/node": "^8.5.8", | |||
There was a problem hiding this comment.
Ideally we could keep these to prevent issues, but if there's a solid reason for moving then 👍
There was a problem hiding this comment.
I think the issue is that Fastify works with Node 4, 6, 8 and 9. Fixing @types/node to a specific version is very confusing for a library.
|
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. |
Me and @mcollina think that the types should stay inside the repo and shipped with the project.
We all discussed a lot in #649 about this, and this is our proposal.
Again, this is the best that thing we can offer, if someone does not agree with this, then he/she/them can start to contribute to the project and take care of this.
I've also moved the
@typesdependencies as dev dependencies, because it is not a good idea ship them directly as dependency. Then I added the@typesdependencies inside the greenkeeper ignore, because every time there is a new release of the types for some unknown reason our CI will complain.cc @fastify/fastify
Checklist
npm run testandnpm run benchmark