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

fix!: drop requirement on URL/combine url and params #338

Merged
merged 3 commits into from
Sep 14, 2020
Merged

fix!: drop requirement on URL/combine url and params #338

merged 3 commits into from
Sep 14, 2020

Conversation

JustinBeckwith
Copy link
Contributor

@JustinBeckwith JustinBeckwith commented Sep 13, 2020

This is a high risk change, please review carefully

I think this would fix #331, and fix #320. We currently use URL to parse the provided url, and then re-build the whole thing after merging querystring parameters. This was causing some issues for folks in #331 where the querystring parser doesn't really understand querystring properties with no values, and it was causing problems in #320 where CloudFlare Workers appear to not have a native URL available to them? That's neat!

This works around both problems by removing the use of URL, and doing a little hand crafting of the querystring. Right now I have this as a fix... I am open to conversations about the semverity of it though.

@JustinBeckwith JustinBeckwith requested a review from a team as a code owner September 13, 2020 21:00
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 13, 2020
@codecov
Copy link

codecov bot commented Sep 13, 2020

Codecov Report

Merging #338 into master will increase coverage by 0.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #338      +/-   ##
==========================================
+ Coverage   95.53%   95.77%   +0.24%     
==========================================
  Files           6        6              
  Lines         672      663       -9     
  Branches      106      109       +3     
==========================================
- Hits          642      635       -7     
+ Misses         30       28       -2     
Impacted Files Coverage Δ
src/gaxios.ts 98.18% <100.00%> (+0.64%) ⬆️

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 aefaad1...cab1b55. Read the comment docs.

@@ -184,7 +195,7 @@ describe('🥁 configuration options', () => {
url: `${url}/?james=beckwith&montgomery=scott`,
params: {james: 'kirk'},
};
const path = '/?james=kirk&montgomery=scott';
const path = '/?james=beckwith&montgomery=scott&james=kirk';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bcoe this is the part of the change that wigs me out a bit. Previously, if you defined a querystring parameter both in the url and in params, we would only ever use one of them when forming the querystring. This strikes me as being ... incorrect. But it may be one of those bugs folks are relying on 🤷 My suspicion is that the risk of breaking runtime behavior is relatively low, because who relies on passing querystring parameters both in the url and in a params object.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to do a quick review of axios, request, and fetch, see how they approach this -- I'll do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have tested axios and request, and convinced myself this is the right behavior.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

I believe this to be the right behavior, but I think we should consider taking it as a breaking change.

The danger I'm mainly worried about is the case where someone has done this:

?scopes=[some-scope]

For url, and also sets params = {scopes: [some other scope]}.

@bcoe bcoe changed the title fix: drop requirement on URL fix!: drop requirement on URL/combine url and params Sep 14, 2020
@bcoe bcoe merged commit e166bc6 into master Sep 14, 2020
@bcoe bcoe deleted the nourl branch September 14, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
2 participants