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

Keep query params when using proxy #17

Merged
merged 1 commit into from
Mar 17, 2017
Merged

Keep query params when using proxy #17

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/jspm-hmr-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}`;
Copy link
Owner

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?

Copy link
Owner

@piotrwitek piotrwitek Mar 17, 2017 •
edited
Loading

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

Copy link
Owner

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?

Copy link
Contributor Author

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!

Copy link
Owner

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!

Copy link
Owner

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 :)

proxy.web(req, res, { target: proxyTarget });
proxy.on('error', (err: Error) => {
console.log('Proxy Server Error: ', err.message);
Expand Down