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

refactor: add query alias to params option #105

Merged
merged 9 commits into from
Sep 19, 2022

Conversation

huynl-96
Copy link
Contributor

@huynl-96 huynl-96 commented Jul 6, 2022

Resolves #73

@pi0
Copy link
Member

pi0 commented Jul 6, 2022

Thanks for PR @huynl-96. I think we can introduce query and deprecate params without breaking changes and it is good to go.

@huynl-96
Copy link
Contributor Author

huynl-96 commented Jul 7, 2022

@pi0 I've added a deprecated warning for params. Thanks for your advice

@pi0 pi0 changed the title refactor(fetch): rename params to query refactor: rename params to query with backward compatibility Sep 19, 2022
@pi0 pi0 changed the title refactor: rename params to query with backward compatibility refactor: add query alias to params option Sep 19, 2022
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #105 (6f60f0e) into main (c1f8d0f) will increase coverage by 0.04%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
+ Coverage   85.43%   85.47%   +0.04%     
==========================================
  Files           5        5              
  Lines         302      303       +1     
  Branches       60       60              
==========================================
+ Hits          258      259       +1     
  Misses         44       44              
Impacted Files Coverage Δ
src/fetch.ts 87.29% <66.66%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pi0
Copy link
Member

pi0 commented Sep 19, 2022

Hi @huynl-96. Sorry it took long for me back to this. I had been thinking it might be better to support both options as alias (without deprecation) because the same "query string" is also named "URL search params" in other standards. Also using "Query search params" in docs to make it readable in both ways.

@pi0 pi0 merged commit 363339d into unjs:main Sep 19, 2022
@huynl-96 huynl-96 deleted the refactor/params-to-query branch September 19, 2022 10:57
@huynl-96
Copy link
Contributor Author

@pi0

Sorry it took long for me back to this

Don't worry about it, hope you are feeling better now.
It's good to finally have you back with us

I had been thinking it might be better to support both options as alias (without deprecation) because the same "query string" is also named "URL search params" in other standards. Also using "Query search params" in docs to make it readable in both ways.

Looks good to me, no concern about it

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.

rename params to query
2 participants