TypeScript Refactor

[Ethan-Arrowood]

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows Code of conduct

I finally feel ready to share my TypeScript refactor ✨ Still WIP and I'll use this PR to track what needs to be down. I'm open to taking other's contributions if they would like work on this with me (this would require making a PR to my personal fork:branch). Ref #1523 for history

TODO:

• Update light-my-request pending Add TypeScript typings light-my-request#39 for inject types
• Type FastifyLogger
• Type FastifySchema
• Type FastifyContext
• Complete migrate original types
• Verify proper export of all types
• Write tests using tsd
• Type ServerFactory
• Document

Some additional context. The fundamental change here is the way I handle the raw HTTP/S/2 server, request, and reply. This is treated as a generic that is passed down through instances of interfaces and types. Additionally, I am using the type T<G> = G && { } to create a generic inheritance type for the FastifyRequest and FastifyReply interfaces. The core generic logic is the following:

declare function fastify<
  RawServer extends http.Server | https.Server | http2.Http2Server | http2.Http2SecureServer = http.Server
>(opts?: fastify.ServerOptions<RawServer>): fastify.FastifyInstance<RawServer>;

declare namespace fastify {
  interface FastifyInstance<
    RawServer extends (http.Server | https.Server | http2.Http2Server | http2.Http2SecureServer) = http.Server, 
    RawRequest extends (http.IncomingMessage | http2.Http2ServerRequest) = RawServer extends http.Server | https.Server ? http.IncomingMessage : http2.Http2ServerRequest, 
    RawReply extends (http.ServerResponse | http2.Http2ServerResponse) = RawServer extends http.Server | https.Server ? http.ServerResponse : http2.Http2ServerResponse
  > { }
}

It starts in the function declaration for fastify main build method. Using TS, the developer would specify which kind of server they are using (it defaults to http). Then, this RawServer type is passed down through to various interfaces such as FastifyInstance and ServerOptions where it utilizes two TS language features: generic constraints and conditional generic defaults.

Generic Constraints (which is actually being used in the first function declaration) is limiting the RawServer generic to be of one of the 4 listed types. Furthermore, the = http.Server is a generic default assignment; this means that the user can define the function without passing in a type to the generic i.e. both of these assignments are valid:

import fastify from 'fastify'
import * as http from 'http'

const server = fastify<http.Server>()
const server = fastify()

Conditional Generic Defaults is a really unique feature that allows us to default the assignment of the other generic variables based on the assignment of the RawServer. This enforces http users to use http request and response objects. Similar effects for https and http2 constraints.

So all in all, when a user first creates their fastify instance, they only have to pass in the server type and then the rest is magically assigned for them. But, at the same time, all of these interfaces and such are exported so if a user really wants to specify a unique FastifyWhatever type they can do that!

[tests-checker]

Hello! Thank you for contributing!
It appears that you have changed the framework code, but the tests that verify your change are missing. Could you please add them?

[mcollina]

light-my-request 3.3.0 is out.

[Ethan-Arrowood]

@mcollina on a previous PR (#1532) you said that we do not want to ship @types/pino from Fastify v2. Considering the default logger in Fastify v2 is dependent on pino, how would you expect me to type this?

This is similar to the recently fixed issue in fastify-jwt (fastify/fastify-jwt#51) where I had to ship jsonwebtoken types as a dep because the index.d.ts file in that project is relying on that packages types.

Let me know what you think!

[evanshortiss]

This looks great! Just skimmed it and adding some thoughts.

[mcollina]

First, the logger can be provided by a user that conform to the same interface. Second, no one from the pino team is involved in maintaining that module. Thirdly, the api surface of jsonwebtoken is minimal compared to a logger - and it is also a very stable module which is essentially “done”.

You might have noticed that my reviews are more strict in certain modules vs others.. mainly because there is a difference. Something that is ok in one, might not be ok in another.

[jsumners]

To support @mcollina's point, this is a valid logger that can be supplied to Fastify:

function log (a, ...args) {
  console.info.apply(console, [a, ...args])
}
const logger = {
  info: log,
  error: log,
  warn: log,
  debug: log,
  fatal: log,
  trace: log,
  child: () => logger
}

[Ethan-Arrowood]

@mcollina sounds good, I'm thinking of a work around that might work for this

@jsumners yep I already have that type of custom logger typed out; its the case of a user using Pino where I'd want them to be able to verify they are passing in valid Pino options to the logger setting on the main fastify object

[Ethan-Arrowood]

My idea for the Pino types is to include this annotated object type that instructs the user how to add Pino typings to their TS project and integrate them with the Fastify types. A user can chose to ignore this and just pass in whatever options object they want, or they can update the type declarations via Declaration Merging. This is in addition to a type intersection of type FastifyLogger = boolean | FastifyLoggerFunction | PinoObject (see logger.d.ts for more details)

[mcollina]

This PR is amazing work, +1.

[Ethan-Arrowood]

@fastify/core what properties exist on the options object in a standard plugin function (not using fastify-plugin)? I.e.

const plugin = function (fastify, options, next) { ... } 
fastify.register(plugin, opts) // the options object on this line. Is it just whatever we wanna pass to the plugin?

Based on avvio I believe these options are open-ended/generic passed in via the user. Which would result in an type declaration of either:

Option 1

export type FastifyPlugin<
  RawServer extends RawServerBase = RawServerDefault,
  RawRequest extends RawRequestBase = RawRequestDefault,
  RawReply extends RawReplyBase = RawReplyDefault
> = (
  instance: FastifyInstance<RawServer, RawRequest, RawReply>,
  opts: { [key as string]: any },
  next: (err?: FastifyError) => void
) => void

const plugin: FastifyPlugin = function (fastify, options, next) { ... }
fastify.register(plugin, opts)
// in this instance `opts` could be an object with any content

or

Option 2

export type FastifyPlugin<
  Options,
  RawServer extends RawServerBase = RawServerDefault,
  RawRequest extends RawRequestBase = RawRequestDefault,
  RawReply extends RawReplyBase = RawReplyDefault
> = (
  instance: FastifyInstance<RawServer, RawRequest, RawReply>,
  opts: Options,
  next: (err?: FastifyError) => void
) => void

// in implementation
interface Options {
  foo: string,
  bar: string
}

const plugin: FastifyPlugin<Options> = function (fastify, options, next) { ... }
const opts: Options = { foo: 'abc', bar: 'xyz' }
fastify.require(plugin, opts)
// so in this example the user can enforce the types for their plugin via an interface and can do so via generics. Furthermore, the `require` method can pass the generic from `plugin` to its own property `opts`. 

[jsumners]

They are whatever the plugin author determines their plugin needs for options.

[Ethan-Arrowood]

I think I have the expected behavior

The second argument in the register function is inferred from the plugin passed in to the first argument! This excites me as I'm using this wonky generic syntax in the FastifyRegister declaration:

export type FastifyRegister<
  RawServer extends RawServerBase = RawServerDefault,
  RawRequest extends RawRequestBase = RawRequestDefault,
  RawReply extends RawReplyBase = RawReplyDefault
> = <Options>(plugin: FastifyPlugin<Options, RawServer>, opts?: RegisterOptions & Options) => void;

export type RegisterOptions = {
  prefix?: string,
  logLevel?: string
}

[Ethan-Arrowood]

@delvedor just to clarify is the work i'm doing here considered a major change right? To start, there are breaking changes in the export structure which effects the way you import the types in TS. Additionally, I've fundamentally changed the server method generics and the way many other methods and interfaces are typed.

[Ethan-Arrowood]

For context on commit 4014e18 the new import structure looks like so:

import fastify, { FastifyPlugin, FastifyInstance } from 'fastify'
// import fastify from 'fastify'
// const { fastify, FastifyPlugin } = require('fastify')
// const fastify = require('fastify')

In these import statements the fastify method can be used as normal i.e. const f = fastify(). If the user wants to strictly declare a type then they are required to use the first or third option to gain access to those specific type declarations.

Does anyone @fastify/core reject this import style for TypeScript users? FWIW all of this and so much more will be accurately documented in the TypeScript page on the Docs site.

[mcollina]

Is that a breaking change?

Also, I’m happy to add some duplication if it makes the life of devs easier. I think exposing the “factory” as a property of itself might be hepful as well.

Wdyt?

[Ethan-Arrowood]

Are you thinking something like import fastify from 'fastify' and then const server = fastify.build() like that?

[jsumners]

For context on commit 4014e18 the new import structure looks like so:

import fastify, { FastifyPlugin, FastifyInstance } from 'fastify'
// import fastify from 'fastify'
// const { fastify, FastifyPlugin } = require('fastify')
// const fastify = require('fastify')

In these import statements the fastify method can be used as normal i.e. const f = fastify(). If the user wants to strictly declare a type then they are required to use the first or third option to gain access to those specific type declarations.

Does anyone @fastify/core reject this import style for TypeScript users? FWIW all of this and so much more will be accurately documented in the TypeScript page on the Docs site.

I have no idea what any of this means. I could not begin to offer an opinion on if it should be rejected or accepted.

[Ethan-Arrowood]

I have no idea what any of this means. I could not begin to offer an opinion on if it should be rejected or accepted.

I'm mainly inquiring about the fact that under TS, users have to import specific types if they want to use them. Fastify has never exported more than just the factory function so I wanted to check that this change was okay and not going against some previous design decision I am unaware of 👍

[mcollina]

I think more supporting

import { fastify, FastifyPlugin, FastifyInstance } from 'fastify'

That will also help esm when it comes out.

[Ethan-Arrowood]

That is easily achievable @mcollina I'll commit that change next and work from there.

[mcollina]

import fastify from 'fastify'

Should still work.

[Ethan-Arrowood]

9dd4a56 ~ 🙌 now supports both~ This is invalid

import { fastify, FastifyInstance, FastifyPlugin} from './fastify'

and

import fastify, { FastifyInstance, FastifyPlugin} from './fastify'

[mcollina]

@Ethan-Arrowood would that work with actual code? I'm not really convinced.

[Ethan-Arrowood]

Actually doesn't work 😅

@mcollina

As far as I can tell it works (if it didn't there would be red squiggles like below)

In this screenshot the export { } were commented out in fastify.d.ts

The type file now exports a couple of things:

import { FastifyInstance } from './types/instance' // import instance interface

export default function fastify< // export default for `import fastify from 'fastify'` syntax
  RawServer extends RawServerBase = RawServerDefault
>(opts?: FastifyServerOptions<RawServer>): FastifyInstance<RawServer>; // use imported interface here

export type FastifyServerOptions<> {} // declare and export the server opts; also used above
export { FastifyPlugin } from './types/plugin' // export the plugin type 
export { FastifyInstance } from './types/instance' // export the instance interface
export { fastify } // export the function so it can be consumed via `import { fastify } from 'fastify'`

[mcollina]

I mean, does it also run? I don’t think we export fastify.fastify in commonjs.

[Ethan-Arrowood]

I mean, does it also run? I don’t think we export fastify.fastify in commonjs.

Oh I see now (import/export is very confusing to me). Okay yes you're right the import { fastify } from 'fastify' is failing when run. Should I modify fastify.js so it exports like that as well or remove that export type from the declaration file?

[tobiasmuehl]

Very excited for this PR, already playing around with it locally.

If a plugin is decorating the FastifyInstance, the decoration still needs to be declared using type augmentation I guess? I tried a type augmentation like in this example with no luck. Now resorting to use lots of // @ts-ignore which is not ideal.

Can you point me in the right direction on how to type plugins that decorate the fastify server?

[Ethan-Arrowood]

New type FastifyContext added. I need some additional clarification by @fastify/core though.

Is it only available on the reply object?
Should it decorate all handlers or only the handler method? (for example preHandler)

[mcollina]

Is it only available on the reply object?

Yes.

Should it decorate all handlers or only the handler method? (for example preHandler)

I don't understand this question. It's available in reply, so it will be available everywhere.

[Ethan-Arrowood]

Should it decorate all handlers or only the handler method? (for example preHandler)

I don't understand this question. It's available in reply, so it will be available everywhere.

I meant the reply object in the handler versus the reply object in the other route methods. But anyways it seems like it should be available on all of them so I'll make the appropriate change in the types and add some additional tests 👍

[mcollina]

The reply object is the same object everywhere.

[Ethan-Arrowood]

Self-reviewed. After I resolve these comments this PR is good to merge.

[Ethan-Arrowood]

Edits from my self review are complete.

Big updates:

• Added a linter for TypeScript (ESLint + Standard config)
• Fixed Hooks (they were totally broken and untested before)
• Added the ability to extend the Header list of FastifyRequest

With that said this PR is finally ready to go -- for real this time.

Important Tomorrow I am leaving for a month and a half trip to Europe. I will not have my computer with me and will only be able to review/discuss from my phone. While I'm gone feel free to merge this, let it sit, or even push updates to the branch to finish it. No matter what I'll be back contributing in full force in mid October!

[mcollina]

@delvedor @evanshortiss @SkeLLLa would you like to give this a final pass before we land?

[SkeLLLa]

@mcollina this PR LGTM.
However, as it was discussed before we'll need to add more ts/js docs later and probably add automatically generated docs as well (after this PR will land).

[mcollina]

Landed!

[delvedor]

Sorry for the delay, amazing work!

[github-actions]

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.