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.

Sign up for GitHub

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

Jump to bottom

Feat: upgrade querystring to URLSearchParams #1574

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

Feat: upgrade querystring to URLSearchParams #1574

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 •
edited

Codecov Report

Merging #1574 (09d7d1f) into master (5ee832c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1574   +/-   ##
=======================================
  Coverage   99.60%   99.60%           
=======================================
  Files           5        5           
  Lines         507      507           
  Branches      142      142           
=======================================
  Hits          505      505           
  Misses          2        2
Impacted Files Coverage Δ
lib/request.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ee832c...09d7d1f. Read the comment docs.

@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 adaex mentioned this pull request Aug 24, 2021
Closed
@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.

miwnwski
miwnwski requested changes Jul 2, 2022
View reviewed changes
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miwnwski miwnwski miwnwski requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Platform: upgrade query string to URLSearchParams
3 participants
@adaex @jonathanong @miwnwski