Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

path can be empty string #110

Merged
merged 2 commits into from
Sep 20, 2016
Merged

path can be empty string #110

merged 2 commits into from
Sep 20, 2016

Conversation

sunnylqm
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Sep 11, 2016

Coverage Status

Coverage decreased (-0.3%) to 96.875% when pulling a3cdd94 on sunnylqm:patch-1 into 7d1865c on chimurai:master.

@chimurai
Copy link
Owner

Please provide a description and test plan for this PR.

@sunnylqm
Copy link
Contributor Author

@chimurai for example

{
        path: '/proxy',
        target: 'http:/example.com',
        pathRewrite: () => ''
}

If I want to replace the whole path into an empty string, then it would complain "No rewritten path found"

@@ -119,7 +119,7 @@ function HttpProxyMiddleware(context, opts) {
if (pathRewriter) {
var path = pathRewriter(req.url, req);

if (path) {
if (path !== false) {
Copy link
Owner

@chimurai chimurai Sep 12, 2016

Choose a reason for hiding this comment

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

Can you change the condition to:

if (typeof path === 'string') {
   ...
}

@chimurai
Copy link
Owner

chimurai commented Sep 12, 2016

Testing for a string should be safer.
This will better cope with 'garbage' returned by custom functions...
(Array's, Objects, undefined, etc...)

@coveralls
Copy link

coveralls commented Sep 13, 2016

Coverage Status

Coverage decreased (-0.3%) to 96.875% when pulling f597c1d on sunnylqm:patch-1 into 7d1865c on chimurai:master.

@sunnylqm
Copy link
Contributor Author

Can it be merged now @chimurai

@chimurai chimurai merged commit 0cb6839 into chimurai:master Sep 20, 2016
@chimurai
Copy link
Owner

Sorry for the late response.
Thanks for contributing!

@sunnylqm sunnylqm deleted the patch-1 branch September 21, 2016 00:30
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants