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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom querystring #1270

Merged
merged 7 commits into from Nov 26, 2018
Merged

Custom querystring #1270

merged 7 commits into from Nov 26, 2018

Conversation

delvedor
Copy link
Member

@delvedor delvedor commented Nov 25, 2018

Implementation of what has been proposed in #1268.

We are also dropping the use of url.parse, so this change will be flagged as semver-major.
With this change, the querystring parsing is twice faster than before 馃殌

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

 - Dropped url.parse API
 - Use the querystring module
 - Exposed a querystring parser option
@delvedor delvedor added feature semver-major Issue or PR that should land as semver major labels Nov 25, 2018
@delvedor delvedor added this to the v2.0.0 milestone Nov 25, 2018
@delvedor delvedor requested a review from a team November 25, 2018 22:53
fastify.js Show resolved Hide resolved
fastify.js Outdated Show resolved Hide resolved
@jsumners
Copy link
Member

Given that this is a semver major, I agree that we should be using the URL function instead of querystring. Better to migrate to the future at once instead of over multiple releases.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

The reason why we are doing this is to remove url.parse. That is the library that is deprecated and should not really be used.

Parsing the querystring via new URL is unfortunately an harder change, as the result is exposed as a Map-like object. We need to pass that into our schema validator, and I fear we cannot use that with some good throughput yet.

(IMHO this is not semver-major, url.parse use querystring internally).

@jsumners
Copy link
Member

Okay.

Copy link
Member

@StarpTech StarpTech left a comment

Choose a reason for hiding this comment

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

LGTM

fastify.js Outdated Show resolved Hide resolved
Copy link
Member

@allevo allevo left a comment

Choose a reason for hiding this comment

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

This feature is not plugin encapsulated. Is there a reason for it?

fastify.js Outdated Show resolved Hide resolved
@delvedor
Copy link
Member Author

This feature is not plugin encapsulated. Is there a reason for it?

  1. We want to ship it in the first RC, add a new API to allow encapsulation could be done in a semver minor feature.
  2. Generally speaking usually the default querystring parser is enough, so we expose a simple way to override it without adding a brand new API.

@mcollina
Copy link
Member

This feature is not plugin encapsulated. Is there a reason for it?

I think there are only so many ways that a querystring could be converted to JS objects. Within a single HTTP server, those should be the same throughout the system. It's kind of the same reason why caseSensitive is in the options https://www.fastify.io/docs/latest/Server/#casesensitive.

We could make this encapsulated, but I'm not convinced it would be worthwhile. I might be wrong.

Copy link
Contributor

@cemremengu cemremengu left a comment

Choose a reason for hiding this comment

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

LGTM very nice feature 馃憤

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM, good work

Copy link
Contributor

@nwoltman nwoltman left a comment

Choose a reason for hiding this comment

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

LGTM

@delvedor delvedor merged commit 5fe52e7 into next Nov 26, 2018
@delvedor delvedor deleted the custom-querystring branch November 26, 2018 17:54
@jannikkeye
Copy link
Contributor

Please update the typings in the future if flags are added. Since the typings are shipped with fastify they should be updated with every change made. I would understand not updating them if types were shipped via @types.

@delvedor
Copy link
Member Author

@jannikkeye pr are welcome!

@jsumners
Copy link
Member

@jannikkeye the Fastify project relies on the TypeScript community to maintain the typings. The majority of the Fastify core team does not know TypeScript.

@0xZaycev
Copy link

0xZaycev commented Jun 9, 2020

thanks for this update)))))))))

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
@Eomm Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-major Issue or PR that should land as semver major semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants