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: add qs codemod #82

Merged
merged 2 commits into from
Aug 26, 2024
Merged

feat: add qs codemod #82

merged 2 commits into from
Aug 26, 2024

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Aug 2, 2024

Adds a codemod for migrating from qs to picoquery.

Some options qs offers are not available. Currently, this mod will not warn on those but should in future.

Draft until we decide these things:

  • should it throw when unknown/unsupported options are used?
  • when encode: false is set, should we replace with decodeURIComponent(stringify(obj))? (encode is a pretty bad option to have so ideally we should just move consumers off it)
  • there are some rarely used options like object sort functions etc that we shouldn't really try codemod. do we just throw when they're used?

@thepassle
Copy link
Collaborator

I think throwing with a clear error message is fine 👍

@PuruVJ
Copy link
Contributor

PuruVJ commented Aug 2, 2024

This is awkward 😅 #83

@thepassle
Copy link
Collaborator

James and I had a long discussion about this on the e18e discord, and we concluded that migrating qs to pq might be the better long term solution for projects. While neoqs is a great drop-in replacement, pq also aligns closer to the URL spec, a lot of work went into the perfomance of it (that's not to say that's not the case for neoqs), and pq also fixes some of the "correctness" or weirdness/quirks of qs. We can discuss more about this on the discord in the #cleanup channel if you like, but let's keep the comments of this PR related to the actual PR 🙂

@PuruVJ
Copy link
Contributor

PuruVJ commented Aug 2, 2024

No worries, if that's the direction we're going with!

@thepassle
Copy link
Collaborator

should it throw when unknown/unsupported options are used?

I think it's fine if we bail on these cases/if we can't codemod it

when encode: false is set, should we replace with decodeURIComponent(stringify(obj))? (encode is a pretty bad option to have so ideally we should just move consumers off it)

Hm that would be a breaking change in behavior, I'm ok with moving consumers off of it, but maybe we should add some logging to let them know about it?

there are some rarely used options like object sort functions etc that we shouldn't really try codemod. do we just throw when they're used?

I think thats fine yeah

@43081j
Copy link
Contributor Author

43081j commented Aug 4, 2024

Hm that would be a breaking change in behavior, I'm ok with moving consumers off of it, but maybe we should add some logging to let them know about it?

i think that would be sensible

the option leads to all sorts of bugs and weird behaviours so we should 100% be moving people off it. ill see if i can get it to wrap with decodeURIComponent then log a warning

@43081j 43081j force-pushed the qs branch 2 times, most recently from d54932d to 8930e34 Compare August 6, 2024 17:36
@43081j
Copy link
Contributor Author

43081j commented Aug 6, 2024

@thepassle i've done the following:

  • reworked the options to be a map of option to Replacer to make things easier
  • added warnings for options we're unaware of (which will be dropped)
  • added a warning for encode: false and wrapped the result in decodeURIComponent automatically

@43081j 43081j marked this pull request as ready for review August 6, 2024 17:38
43081j added 2 commits August 20, 2024 22:10
Adds a codemod for migrating from `qs` to `picoquery`.

Some options qs offers are not available. Currently, this mod will not
warn on those but should in future.
@thepassle thepassle merged commit dcd6b17 into main Aug 26, 2024
3 checks passed
@43081j 43081j deleted the qs branch August 26, 2024 12:49
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