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 common.urlJoin when target has query string #3

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

potoo0
Copy link
Contributor

@potoo0 potoo0 commented Jul 3, 2024

Close: #2

@Jimbly
Copy link
Owner

Jimbly commented Jul 3, 2024

I'm unfamiliar with the workings of this library other than the one bug I fixed, but I guess I'm maintaining it now =). What use case / problem in the public API does this fix? Is this the same bug as https://github.com/http-party/node-http-proxy/pull/1334/files is fixing (and does that 1-line fix also fix your issue)?

@potoo0
Copy link
Contributor Author

potoo0 commented Jul 3, 2024

No, this commit actually fixed two bugs:

  1. do not split url by ?, because the query string may contain ?, eg: git.io/p1?a=a?1?2
  2. fix target path has query string, eg: urlJoin('/p1?a=1', '/p2') expected /p1/p2?a=1, but got /p1?a=1/p2

@Jimbly
Copy link
Owner

Jimbly commented Jul 3, 2024

What use case / problem in the public API does this fix? urlJoin isn't part of the public API, right?

@potoo0
Copy link
Contributor Author

potoo0 commented Jul 3, 2024

I built a gateway that depends on this proxy lib, an error occurs when the user-configured target contains query string.


For this particular proxy case, I will maintain it myself.


case
server:

httpProxy.createProxyServer({target:'http://httpbin.org?token=123'}).listen(8000);

client:

curl http://localhost:8000/get

@potoo0 potoo0 closed this Jul 3, 2024
@Jimbly
Copy link
Owner

Jimbly commented Jul 3, 2024

I'm happy to merge and publish it, just wanted to know what the use case was so I could at least do a quick test that nothing broke/it works =).

@potoo0
Copy link
Contributor Author

potoo0 commented Jul 3, 2024

Ok, close this pr if you don't think it's necessary.

@potoo0 potoo0 reopened this Jul 3, 2024
@Jimbly Jimbly merged commit 846f071 into Jimbly:master Jul 3, 2024
@Jimbly
Copy link
Owner

Jimbly commented Jul 3, 2024

Thanks! Published to NPM as v1.0.4

This pull request was closed.
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.

common.urlJoin got unexpected result
2 participants