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

perf: Switch deprecated parser querystring for fast-querystring #14677

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smith558
Copy link

@smith558 smith558 commented Feb 21, 2025

This should be considered. The performance gains are not negligible. Additionally, find-my-way and Fastify use it by default, so there's no reason to regress on this for the adapter.

The querystring is also absolutely deprecated with the last feature commit from 2018 (it seems). @kamilmysliwiec
image

Related to #10248.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 38450cb8-a4f4-4360-852f-b9183c3e5386

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 89.292%

Totals Coverage Status
Change from base Build 57005691-848e-40a1-8900-5c194e43249e: 0.0%
Covered Lines: 7146
Relevant Lines: 8003

💛 - Coveralls

@kamilmysliwiec
Copy link
Member

image

https://fastify.dev/docs/latest/Reference/Server/#querystringparser

@smith558
Copy link
Author

smith558 commented Feb 22, 2025

image https://fastify.dev/docs/latest/Reference/Server/#querystringparser

This was a mistake in the docs and is being fixed. If you look at the source code it's clear they actually use fast-querystring. You may also look at fastify/fastify#5993.

@smith558
Copy link
Author

This has now been fixed on the Fastify side (fastify/fastify#5993). Let's merge this if you're happy with it. @kamilmysliwiec

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.

3 participants