Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeScript Refactor #1569

Merged
merged 51 commits into from Aug 27, 2019

Conversation

@Ethan-Arrowood
Copy link
Member

commented Apr 4, 2019

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 fastify/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
Copy link

left a comment

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

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

light-my-request 3.3.0 is out.

@Ethan-Arrowood

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@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
Copy link
Member

left a comment

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

types/middleware.d.ts Outdated Show resolved Hide resolved
types/middleware.d.ts Outdated Show resolved Hide resolved
@mcollina

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

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)
Screen Shot 2019-04-05 at 2 21 04 PM

@mcollina

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

This PR is amazing work, +1.

types/route.d.ts Outdated Show resolved Hide resolved
@Ethan-Arrowood

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@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

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

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

@Ethan-Arrowood

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

I think I have the expected behavior
Screen Shot 2019-04-11 at 11 20 39 PM

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

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

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

@jsumners

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

I think more supporting

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

That will also help esm when it comes out.

@Ethan-Arrowood

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

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

@mcollina

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

import fastify from 'fastify'

Should still work.

@Ethan-Arrowood

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

9dd4a56 ~ 🙌 now supports both~ This is invalid

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

and

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

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

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

@Ethan-Arrowood

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Actually doesn't work 😅

@mcollina Screen Shot 2019-04-12 at 10 56 19 AM Screen Shot 2019-04-12 at 10 56 28 AM

As far as I can tell it works (if it didn't there would be red squiggles like below)
Screen Shot 2019-04-12 at 11 07 17 AM
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

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

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

@Ethan-Arrowood

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

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?

types/logger.d.ts Outdated Show resolved Hide resolved
types/register.d.ts Outdated Show resolved Hide resolved
types/schema.d.ts Outdated Show resolved Hide resolved
types/context.d.ts Outdated Show resolved Hide resolved
@evanshortiss

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

incredible work @Ethan-Arrowood. No major issues that I see, just added minor comments.

@tobiasmuehl

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

The reply object is the same object everywhere.

@Ethan-Arrowood
Copy link
Member Author

left a comment

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

fastify.js Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
test/imports.test.js Show resolved Hide resolved
test/types/content-type-parser.test-d.ts Outdated Show resolved Hide resolved
types/hooks.d.ts Show resolved Hide resolved
types/hooks.d.ts Outdated Show resolved Hide resolved
types/instance.d.ts Outdated Show resolved Hide resolved
types/instance.d.ts Outdated Show resolved Hide resolved
types/request.d.ts Outdated Show resolved Hide resolved
@Ethan-Arrowood

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

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

@SkeLLLa

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

@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 mcollina merged commit 2c60388 into fastify:next Aug 27, 2019

20 checks passed

fastify.fastify Build #20190827.2 succeeded
Details
fastify.fastify (Linux_Yarn node_10_x) Linux_Yarn node_10_x succeeded
Details
fastify.fastify (Linux_Yarn node_12_x) Linux_Yarn node_12_x succeeded
Details
fastify.fastify (Linux_Yarn node_8_x) Linux_Yarn node_8_x succeeded
Details
fastify.fastify (Linux_npm node_10_x) Linux_npm node_10_x succeeded
Details
fastify.fastify (Linux_npm node_12_x) Linux_npm node_12_x succeeded
Details
fastify.fastify (Linux_npm node_8_x) Linux_npm node_8_x succeeded
Details
fastify.fastify (Windows_NPM node_10_x) Windows_NPM node_10_x succeeded
Details
fastify.fastify (Windows_NPM node_12_x) Windows_NPM node_12_x succeeded
Details
fastify.fastify (Windows_NPM node_8_x) Windows_NPM node_8_x succeeded
Details
fastify.fastify (Windows_yarm node_10_x) Windows_yarm node_10_x succeeded
Details
fastify.fastify (Windows_yarm node_12_x) Windows_yarm node_12_x succeeded
Details
fastify.fastify (Windows_yarm node_8_x) Windows_yarm node_8_x succeeded
Details
fastify.fastify (macOS_yarn node_10_x) macOS_yarn node_10_x succeeded
Details
fastify.fastify (macOS_yarn node_12_x) macOS_yarn node_12_x succeeded
Details
fastify.fastify (macOS_yarn node_8_x) macOS_yarn node_8_x succeeded
Details
fastify.fastify (macOs_npm node_10_x) macOs_npm node_10_x succeeded
Details
fastify.fastify (macOs_npm node_12_x) macOs_npm node_12_x succeeded
Details
fastify.fastify (macOs_npm node_8_x) macOs_npm node_8_x succeeded
Details
security/snyk - package.json (mcollina) No new issues
Details
@mcollina

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Landed!

@delvedor
Copy link
Member

left a comment

Sorry for the delay, amazing work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.