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: upgrade querystring to URLSearchParams #1574

Closed
wants to merge 1 commit into from

Conversation

adaex
Copy link

@adaex adaex commented Aug 23, 2021

close #1570

I'm not sure if this is necessary.

@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.60%. Comparing base (5ee832c) to head (09d7d1f).
Report is 97 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1574   +/-   ##
=======================================
  Coverage   99.60%   99.60%           
=======================================
  Files           5        5           
  Lines         507      507           
  Branches      142      142           
=======================================
  Hits          505      505           
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonathanong
Copy link
Member

thanks! looking more into this, do you think it would be better if we just exposed the URLSearchParams instance as ctx.request.searchParams or something?

yeah, not necessary, but query string is considered legacy

@adaex
Copy link
Author

adaex commented Aug 24, 2021

@jonathanong

I think exposed the URLSearchParams instance as ctx.request.searchParams is a good idea.

How do you see #1575 ? If there is no problem, I will continue to complete it.

Copy link
Member

@miwnwski miwnwski left a comment

Choose a reason for hiding this comment

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

LGTM, but requires some additions.

If you don't mind exposing searchParams as Request#searchParams, and it would be nice to add a line to History.md as well as this is a breaking change. Also a test or two for completeness!

Since this is available from node 10+ it's mergeable once the PR is massaged a bit.

I closed #1575 in favour of this PR.

@jonathanong
Copy link
Member

merged #1828

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.

Platform: upgrade query string to URLSearchParams
3 participants