docs(examples): Fix ineffective TypeScript example#350
docs(examples): Fix ineffective TypeScript example#350smith558 wants to merge 2 commits intofastify:mainfrom
Conversation
Signed-off-by: Stanislav (Stanley) Modrak <44023416+smith558@users.noreply.github.com>
Signed-off-by: Stanislav (Stanley) Modrak <44023416+smith558@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR updates the TypeScript example in the QuickStart component to improve type safety and add server startup logging.
- Added explicit generic type parameters to
FastifyInstancefor better TypeScript type safety - Enabled logger in the Fastify initialization
- Added server startup log message to inform users when the server is listening
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { Server, IncomingMessage, ServerResponse } from 'http' | ||
|
|
||
| const server: FastifyInstance = Fastify({}) | ||
| const server: FastifyInstance<Server, IncomingMessage, ServerResponse> = Fastify({ logger: true }) |
There was a problem hiding this comment.
The explicit generic type parameters <Server, IncomingMessage, ServerResponse> are unnecessary in modern Fastify TypeScript usage. Fastify v4+ has improved type inference and these generics are deprecated. The simpler const server: FastifyInstance = Fastify({ logger: true }) is the recommended approach.
There was a problem hiding this comment.
@Eomm I know that's the case (the types are inferred since v4+). But this code block is meant to be a trivial code example. Of which the whole purpose was just to demonstrate TypeScript typing compatibility of Fastify. This is still hugely better than it was before, which had floating unused imports appearing in a code block, with no apparent use (and breaking TypeScript lint rules). As well as the accompanying text not logically relating to the code example. Otherwise, the whole code block should just be removed completely (as it doesn't demonstrate anything and actually breaks good practices).
I am not sure how much Copilot feedback is relevant here as this is meant to be a training article. It likely misses that the point here is "demonstration".
There was a problem hiding this comment.
It may be useful to add a comment line // from Fastify v4 these types are inferred automatically, you may omit them
There was a problem hiding this comment.
@Eomm btw, Copilot is wrong here, this generic is not deprecated, it's just inferred and not necessary to type in, but the type generic is still used and is very much valid
|
|
||
| const address = server.server.address() | ||
| const port = typeof address === 'string' ? address : address?.port | ||
| server.log.info(`Server listening on port ${port}`) |
There was a problem hiding this comment.
Error: Unexpected token
|
|
||
| const address = server.server.address() | ||
| const port = typeof address === 'string' ? address : address?.port | ||
| server.log.info(`Server listening on port ${port}`) |
There was a problem hiding this comment.
| server.log.info(`Server listening on port ${port}`) | |
| server.log.info('Server listening on port %s', port) |
There was a problem hiding this comment.
Are you sure? All other examples in the docs prefer to use ${x} type of interpolation (e.g. https://fastify.dev/docs/latest/Guides/Getting-Started/). We should prefer consistency.
| import { Server, IncomingMessage, ServerResponse } from 'http' | ||
|
|
||
| const server: FastifyInstance = Fastify({}) | ||
| const server: FastifyInstance<Server, IncomingMessage, ServerResponse> = Fastify({ logger: true }) |
|
@mcollina thoughts? |
|
Hi. Are you please interested in this change? @Eomm The current Typescript example in the docs is truly broken and will only discourage people from using Fastify, if they care about Typescript. I am happy to rework the example in another way. |
Please @fastify/typescript could you check this PR and take a decision? To answer you: I'm not a TS user, from my POV: if there are 10 TS devs, there are 10 different ways of configuring types and I really don't know which one should be added in the homepage. |
Description
This fixes the currently very ineffective and (imo) broken TypeScript use example.
Issues addressed:
portto show how one would get the portold version:
Related Issues
Check List