-
Notifications
You must be signed in to change notification settings - Fork 6
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
Keep query params when using proxy #17
Conversation
When using a proxy, any query-parameters are lost.
@@ -66,7 +66,7 @@ export function createServer(options: ServerOptions): JspmHmrServer { | |||
|
|||
const proxy = httpProxy.createProxyServer(); | |||
app.use(proxyRoute, (req, res) => { | |||
req.url = req.baseUrl; | |||
req.url = `${req.baseUrl}${req.url}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for cases that have requests only for base route e.g. /search?q=something
However I'm not sure it would work for deeper routes like e.g. /api/products/search?q=something
, can you please confirm that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because proxyRoute
option param above is selecting mountpath for proxy, so I'm wondering if that has any impact in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be honest I'm not even sure why this line is there maybe we can remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my case req.baseUrl
contains /api/some-service/some-entity/find
and req.url
contains /?parameter=test
. Concatenating them seems to yield the correct address. I've tested it with urls without any parameters as well.
If I remove the line the url only consists /?parameter=test
and fails!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amygdaloideum thanks, looks good to me 👍
I was planning to write some tests for basic features, I gonna use this as a test case
good job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amygdaloideum added test coverage for your case here: #18
confirmed to be working as expected :)
When using a proxy, any query-parameters are lost.