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

feat(platform-fastify): added compatibility for fastify version 5.x 🔥 #13990

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Tony133
Copy link
Contributor

@Tony133 Tony133 commented Sep 17, 2024

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
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Proposal:

  • Added compatibility for Fastify version 5.x 🔥

Does this PR introduce a breaking change?

  • [] Yes
  • [] No

Other information

It would be nice if NestJS version 10.x was already compatible with Fastify v5.x without waiting for the next major version of NestJS, leaving compatibility with Fastify v4.x

@coveralls
Copy link

coveralls commented Sep 17, 2024

Pull Request Test Coverage Report for Build 7e0bb406-6bd1-460e-b3f8-f7d31a089897

Details

  • 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 92.227%

Totals Coverage Status
Change from base Build 2bc2268b-f14e-423e-ae37-e98ea536e4b2: 0.0%
Covered Lines: 6751
Relevant Lines: 7320

💛 - Coveralls

@Tony133 Tony133 changed the title feat: upgrade fastify v5.x feat: added compatibility for Fastify version 5.x 🔥 Sep 17, 2024
@Tony133 Tony133 changed the title feat: added compatibility for Fastify version 5.x 🔥 feat: added compatibility for fastify version 5.x 🔥 Sep 17, 2024
@Tony133 Tony133 changed the title feat: added compatibility for fastify version 5.x 🔥 feat(platform-fastify): added compatibility for fastify version 5.x 🔥 Sep 17, 2024
@micalevisk
Copy link
Member

@kamilmysliwiec
Copy link
Member

Fastify v5 requires Node v20 so we will have to wait till the next major release of Nest to make the version bump.

@micalevisk
Copy link
Member

@johaven please open a new feature request issue for that instead.

@songkeys
Copy link

Fastify v5 requires Node v20 so we will have to wait till the next major release of Nest to make the version bump.

If it's compatible with both v4 and v5, I don't think it's necessary to wait for the next major of Nest?

@zackdotcomputer
Copy link

Agreed with @songkeys - since this is just expanding support to include Fastify 5, and not dropping support for earlier versions either of fastify or of node itself, it feels like it could be included in a minor version update?

@SzymonGonet
Copy link

SzymonGonet commented Oct 10, 2024

@kamilmysliwiec I guess there’s no need to wait for the next major release - if possible, it’d be great if you could go ahead and merge it.

@micalevisk
Copy link
Member

micalevisk commented Oct 14, 2024

The thing is that: NestJS v10 is supposed to support nodejs v16. Thus, changing those semver ranges of dependencies list will break this support because Fastify v5 only supports Node.js v20+. So running npm i @nestjs/platform-fastify (without having fastify in your deps list) will install fastify@5 alongside. And you won't see anything wrong after installing it because fastify don't have the engines.node attribute on their package.json.

I know that if you have fastify@4 in your dependencies list already, the v5 one won't be installed upon installing @nestjs/platform-fastify

Not sure how we could address this limitation(?) without a major bump on NestJS side.

@zackdotcomputer
Copy link

Ah - that makes sense @micalevisk. I actually ran into that issue with a different package recently so understand what you mean. One possibility for future-proofing this would be to move fastify to be a peer dependency, so that end-developers manage the version of fastify they're using directly? That sort of change would also definitely need to be done in a major version bump, though.

I guess, given that, what are the possibilities of doing a major version bump for the nestjs + fastify package specifically? Or is the philosophy here that this package's version needs to stay in sync with the mainline nestjs?

@micalevisk micalevisk mentioned this pull request Oct 16, 2024
1 task
@PattyTrish
Copy link
Contributor

I'd like to see if we could have a major version in the works to support both Fastify v5 and Express v5, since these have both just come out recently.

Due to the version ambiguity @micalevisk mentioned, I think a major version update is the way to go, FWIW. Is there ongoing work/planning for this? The packages here all leave LTS next year: it needs to happen at some point.

@gperdomor
Copy link
Contributor

The thing is that: NestJS v10 is supposed to support nodejs v16. Thus, changing those semver ranges of dependencies list will break this support because Fastify v5 only supports Node.js v20+. So running npm i @nestjs/platform-fastify (without having fastify in your deps list) will install fastify@5 alongside. And you won't see anything wrong after installing it because fastify don't have the engines.node attribute on their package.json.

I know that if you have fastify@4 in your dependencies list already, the v5 one won't be installed upon installing @nestjs/platform-fastify

Not sure how we could address this limitation(?) without a major bump on NestJS side.

In any case, Node 16 reach end of life1 year ago, and Node 18 will do the same in 6 months, so I think that release a new major release with Node 20 as breaking change is more that justified...

Copy link

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FastifyReply added RouteGeneric before the RawServer generic (following suit of FastifyRequest).
This needs to be accounted for in the adapter.
https://github.com/nestjs/nest/blob/master/packages/platform-fastify/adapters/fastify-adapter.ts#L131-L132

Also the path-to-regexp v6 -> v8 is breaking the path normalization in the middleware.
https://github.com/nestjs/nest/blob/master/packages/platform-fastify/adapters/fastify-adapter.ts#L584-L600

@gperdomor
Copy link
Contributor

Any progress here?... :(

@micalevisk
Copy link
Member

@gperdomor the support for Fastify v5 will be add in NestJS v11. There is no ETA.

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.

10 participants