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

Replace qs with neoqs #88

Open
wants to merge 1 commit into
base: 7.x
Choose a base branch
from
Open

Replace qs with neoqs #88

wants to merge 1 commit into from

Conversation

RomainLanz
Copy link
Member

@RomainLanz RomainLanz commented Jul 25, 2024

Hey there! 👋🏻

This PR replaces the usage of qs for neoqs.

neoqs is a drop-in replacement for qs without the bloat.

Moving to it means:

  • We don't care about Node.js 0.6
  • It is 3x smaller
  • It has zero dependencies
  • It is built for ESM
  • TypeScript types are backed-in

QS Dependencies

qs dependencies
CleanShot 2024-07-25 at 08 18 38@2x

NeoQS Dependencies

Neoqs dependencies
CleanShot 2024-07-25 at 08 19 14@2x

@thetutlage
Copy link
Member

Looks nice. But, I think I will inspect the neoqs package a bit and run some tests on it. The package has zero dependents and hardly any installs :)

@RomainLanz
Copy link
Member Author

Looks nice. But, I think I will inspect the neoqs package a bit and run some tests on it. The package has zero dependents and hardly any installs :)

It is super new. It is part of an initiative to debloat the Node ecosystem by cleaning packages from "we know who". 😄

I was planning to rewrite qs in BoringNode, but since the community already does it, I believe it is unnecessary. 👍🏻

@thetutlage
Copy link
Member

It is super new

Yeah, that's why want to run a few tests myself. Since, the query string parsing it pretty much used by every AdonisJS app running in prod. I want to make sure we do not break anything (even by mistake) :)

@PuruVJ
Copy link

PuruVJ commented Aug 2, 2024

Hi @thetutlage! Creator of neoqs here. If you have any questions you'd like to ask me, feel free to do so, and I'll do my best to answer them!

@@ -7,7 +7,7 @@
* file that was distributed with this source code.
*/

import { parse, stringify } from 'qs'
import { parse, stringify } from 'neoqs'
Copy link

Choose a reason for hiding this comment

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

If this package still ships CommonJS in any capacity, this should be neoqs/legacy. Same for the other file

@RomainLanz
Copy link
Member Author

We discussed this internally, and we would prefer not to migrate to neoqs as of now. But we keep the PR open for further discussion.

The main reason is that qs is a critical part of AdonisJS, and this new package may not fit all use cases or keep up with the qs package.

Also, migrating to a brand new package may add more vulnerability since we don't know well the author nor its future intention.

If we want to debloat qs, we would prefer to make our own version under the BoringNode organization.

Related PRs are:

@PuruVJ
Copy link

PuruVJ commented Aug 12, 2024

Thanks a lot for the response! As the author of the package, I'd like to address some of your concerns:

The main reason is that qs is a critical part of AdonisJS, and this new package may not fit all use cases or keep up with the qs package.

I am tracking qs for every release. Whenever qs cuts a release, I'll backport the same code to neoqs, so it'll always be up to date(barring a few days to a week based on my bandwidth). Same for all the tests, neoqs won't cut a release if all the tests don't pass the qs tests.

Also, migrating to a brand new package may add more vulnerability since we don't know well the author nor its future intention.

This is understandable. However I'd like to point out that qs has 14 transitive dependency, and any one of them going rogue/being compromised poses a larger risk, compared to neoqs which has 0 dependencies.

As for the package's intentions, it is to stay 0-dependency forever, and modernise overtime to cut down the bloat. neoqs/legacy is there to cater to very old versions(Node 8+ and any other ES5-only browsers). Moresoever, neoqs is published with provenance, and signed by github, and commits are tracked. This is in case the package gets compromised and published by someone other than me, it'll breach the provenance contract. It's only a tiny bit extra security, but it's there nonetheless.

CleanShot 2024-08-12 at 14 32 30

If we want to debloat qs, we would prefer to make our own version under the BoringNode organization.

I respect that! Feel free to copy https://github.com/PuruVJ/neoqs/tree/main/src to ur own package with proper attribution.

Have a good day

@thetutlage
Copy link
Member

Hello @PuruVJ

Thanks for taking the feedback positively. One quick question. Do you plan to maintain the feature set of qs in the future? Or are you planning to eventually make neoqs simpler overtime by removing features?

@PuruVJ
Copy link

PuruVJ commented Aug 12, 2024

Thanks for the quick reply!

I won't remove any features of my own accord. If qs adds a feature, I will add it. if qs removes a feature, I will remove it as well.

@thetutlage
Copy link
Member

@PuruVJ Good to know :)

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