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

refactor(platform-fastify): supports fastify 3.0 #5070

Merged
merged 8 commits into from
Jul 29, 2020

Conversation

hongyiweiwu
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Other... Please describe: Supports for Fastify 3.0. Minor API breaks.

What is the current behavior?

Current platform-fastify package only supports Fastify 2.0.

What is the new behavior?

The package is fully Fastify 3.0 compatible. In addition, the following non-breaking changes are made:

  • The adapter now takes three type arguments (all optional) TServer, TRawRequest, TRawResponse that make the type suggestions clearer.
  • useStaticAssets originally took a subset of options available to the Fastify instance function with the same name. It now takes the same options interface.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

The following breaking changes are not really breaking if you were using the package correctly before.

  • The following functions originally had any as the type for its first argument res. Now the types are specified to FastifyReply (or FastifyReply | TRawResponse if native response is allowed) : reply, status, render, redirect, setHeader, getRequestHostname, getRequestMethod, getRequestUrl.

The following changes are breaking: some come from changes within Fastify itself; others are package cleanup.

  • The following functions originally took in variadic arguments of type any. They're now changed into the same signature as the Fastify instance function with the same name: register, inject,

  • The following functions originally took additional arguments that were not used. These arguments are now removed: setErrorHandler, setNotFoundHandler, initHttpServer.

  • setViewEngine originally typed its argument as any. This is because the appropriate Fastify plugin invoked in this function takes in a PointOfViewOptions argument, but in the base HTTP adapter, the function setViewEngine takes in a string. So far I allow the argument type to be PointOfViewOptions | string and then does a runtime check, throws error and exits if the argument type is string. This works though it's quite ugly: maybe something should be done on the base HTTP adapter level.

@hongyiweiwu hongyiweiwu mentioned this pull request Jul 11, 2020
@coveralls
Copy link

coveralls commented Jul 11, 2020

Pull Request Test Coverage Report for Build 23f04693-e216-4d8c-ac60-47e1e3cf9ce3

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.817%

Totals Coverage Status
Change from base Build 0934aae2-2490-4c65-9fda-1dd74b49a5e6: 0.0%
Covered Lines: 4921
Relevant Lines: 5190

💛 - Coveralls

@hongyiweiwu
Copy link
Contributor Author

We might need to introduce a lot more breaking changes than anticipated. This is mainly because in Fastify 3.0 official support for Express-style middlewares are dropped and we need to use the plugin fastify-express as a compatibility layer. Since Fastify loads plugins asynchronously, the createMiddlewareFactory method needs to return an async function that will be awaited on. We then need to await all calls to this function, as well as redefining the signature for HttpServer, which breaks the express adapter and all custom adapters.

This will likely result in breaking changes in core, common, platform-fastify and platform-express. Should I just bump the versions up to 8.0.0 for these four? @kamilmysliwiec

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Jul 16, 2020

We might need to introduce a lot more breaking changes than anticipated. This is mainly because in Fastify 3.0 official support for Express-style middlewares are dropped and we need to use the plugin fastify-express as a compatibility layer. Since Fastify loads plugins asynchronously, the createMiddlewareFactory method needs to return an async function that will be awaited on. We then need to await all calls to this function, as well as redefining the signature for HttpServer, which breaks the express adapter and all custom adapters.

We should be able to just register the middie plugin https://github.com/fastify/middie upon the initialization of the adapter. For this, we could add an empty init() method to the AbstractHttpAdapter (and override it within the FastifyAdapter) and just call it from the core (unless I've missed something)

@hongyiweiwu
Copy link
Contributor Author

https://github.com/fastify/middie

Sure, I'll try this and send a new PR this weekend.

@hongyiweiwu
Copy link
Contributor Author

We should be able to just register the middie plugin https://github.com/fastify/middie upon the initialization of the adapter. For this, we could add an empty init() method to the AbstractHttpAdapter (and override it within the FastifyAdapter) and just call it from the core (unless I've missed something)

In this case we might need to introduce a breaking change in testing, specifically TestingModule.createNestApplication must be made async -- or maybe I'm missing a better way to handle this?

@gperdomor
Copy link
Contributor

We might need to introduce a lot more breaking changes than anticipated. This is mainly because in Fastify 3.0 official support for Express-style middlewares are dropped and we need to use the plugin fastify-express as a compatibility layer. Since Fastify loads plugins asynchronously, the createMiddlewareFactory method needs to return an async function that will be awaited on. We then need to await all calls to this function, as well as redefining the signature for HttpServer, which breaks the express adapter and all custom adapters.

We should be able to just register the middie plugin https://github.com/fastify/middie upon the initialization of the adapter. For this, we could add an empty init() method to the AbstractHttpAdapter (and override it within the FastifyAdapter) and just call it from the core (unless I've missed something)

Why https://github.com/fastify/middie instead https://github.com/fastify/fastify-express ?

@hongyiweiwu
Copy link
Contributor Author

We might need to introduce a lot more breaking changes than anticipated. This is mainly because in Fastify 3.0 official support for Express-style middlewares are dropped and we need to use the plugin fastify-express as a compatibility layer. Since Fastify loads plugins asynchronously, the createMiddlewareFactory method needs to return an async function that will be awaited on. We then need to await all calls to this function, as well as redefining the signature for HttpServer, which breaks the express adapter and all custom adapters.

We should be able to just register the middie plugin https://github.com/fastify/middie upon the initialization of the adapter. For this, we could add an empty init() method to the AbstractHttpAdapter (and override it within the FastifyAdapter) and just call it from the core (unless I've missed something)

Why https://github.com/fastify/middie instead https://github.com/fastify/fastify-express ?

both work :) I'm using fastify-express in my current version but they should have the same functionality. The key issue right now is whether to allow breaking changes inside TestingModule.

@gperdomor
Copy link
Contributor

@hongyiweiwu since fastify-express seems to be the recommended way for fastify 3 according the release notes https://github.com/fastify/fastify/releases/tag/v3.0.0, I recommend use that package :D

@AliYusuf95
Copy link

AliYusuf95 commented Jul 20, 2020

@gperdomor I think middie is better option since they mentioned both. Also, I noted that fastify-express use express instance to run middleware while middie is not.

The point from using Fastify adaper is to deliver better performance, it's bad idea to run all middleware in express instance.

Maybe this will be braking change if the previous version of fastify used express instance to run middleware is this case and to avoid that using fastify-express is more suitable.

@numToStr
Copy link

I agree with @AliYusuf95 as middie would be the better option here instead fastify-express. But I guess this would bring a lot of breaking changes but this should be worth it.

IMO fastify ecosystem is matured enough to replace express plugins. Most of the plugins are already available in fastify ecosystem i.e cors, helmet, IDK about passport though, with better performace.

Nest v8 might be the version with already great DX of nestjs and with insane performace of fastify v3.

@kamilmysliwiec
Copy link
Member

But I guess this would bring a lot of breaking changes but this should be worth it.

Using middie shouldn't introduce any breaking changes. Fastify was never fully compatible with Express.

@numToStr
Copy link

But I guess this would bring a lot of breaking changes but this should be worth it.

Using middie shouldn't introduce any breaking changes. Fastify was never fully compatible with Express.

I suppose then middie is the way ahead?

@hongyiweiwu
Copy link
Contributor Author

Using middie shouldn't introduce any breaking changes. Fastify was never fully compatible with Express.

Still not sure how using middie will avoid breaking changes -- we have to async load the plugin, which will break things.

@kamilmysliwiec kamilmysliwiec added this to the 7.4.0 milestone Jul 29, 2020
@kamilmysliwiec kamilmysliwiec merged commit 0006d31 into nestjs:master Jul 29, 2020
@hongyiweiwu
Copy link
Contributor Author

@kamilmysliwiec This implementation isn't workable yet, because I'm hoping to hear more of your input on how to handle the breaking change in the testing module. So we shouldn't merge it yet. Currently it may break many code.

@kamilmysliwiec
Copy link
Member

I've changed several things in this commit (which should make it workable). Please, check them out here 0006d31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants