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: migrate to picoquery #2146

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

feat: migrate to picoquery #2146

wants to merge 1 commit into from

Conversation

43081j
Copy link

@43081j 43081j commented Jul 27, 2024

Migrates from qs to picoquery for query strings, a faster and lighter alternative.

This is an alternative to #2116 - using pq to provide the logic rather than rolling our own. We are doing the same in storybook and many other repos, so figured it'd be worth doing so here too.

if you'd prefer to roll your own, feel free to close this

Migrates from qs to picoquery for query strings, a faster and lighter alternative.
@CLAassistant
Copy link

CLAassistant commented Jul 27, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@@ -3,6 +3,7 @@
"compilerOptions": {
"outDir": "./esm",
"module": "es2022",
"moduleResolution": "node",
Copy link
Author

Choose a reason for hiding this comment

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

locally the esm tsc build failed without this. i'm not sure how it ever worked without it 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

We have been using the default and have not had issues, could you pull this out of this PR?

@helenye-stripe helenye-stripe self-assigned this Aug 12, 2024
@helenye-stripe
Copy link
Contributor

Hi @43081j, thanks so much for this submission! Sorry for the delay. I think after reviewing the approaches for removing qs, we would like to investigate this further. Would you mind investigating the test failure here?

@43081j
Copy link
Author

43081j commented Aug 18, 2024

unfortunately this is because node 12 doesn't support nullish coalescing

bumping the engine constraint here (i.e. dropping node <14 support) seems the sensible thing to do eventually but that seems like a bigger decision for you to make

until then i guess this PR is blocked

let me know what you think. if we have to hold off until node <14 support is dropped one day, im happy to come back at that point and revisit this

@xavdid-stripe
Copy link
Member

Yes, our version support policy is something we're working on. We'll put a pin in this and merge it when we can drop node 12 support.

Thank you for your contribution!

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

Successfully merging this pull request may close these issues.

4 participants