actual fix for issue #43

[jlank]

reverted slashJoin function, the faulty logic was outside the function, I added a new case check for req.url URLs that contain '/?query=param' as well as '?query=param', tests pass. Closes #43 after merge, bump, and publish.

[andrewrk]

can you add a test to make sure it works?

[jlank]

Yeah good call. Will get one written up.

[jlank]

@andrewrk so I spent a couple more hours on this and I have to apologize. The error I was tracking down was due to an improper invocation of your library in a downstream module. It was causing the proxy to append a trailing slash on mounted routes. My initial PR doesn't change the functionality, so you can either leave the code as-is or revert my changes. Sorry again for the confusion.

[jlank]

Ok, so I think I'm inadvertently trying to set a new record for the amount of times I can stick my foot in my mouth. Here's the next, and hopefully final, double back. I'm about to push up a new code change to slashJoin - this should resolve the issue I was experiencing. I'll do my best to explain what was happening, this is a bit confusing.

I was indirectly leveraging node-proxy-middleware from hiddentao/gulp-server-livereload. When I did this, I was using the following settings:

proxies: [{ source: '/search', target: 'http://localhost:3001/search' }]

which in node-proxy-middleware was being applied in gulp-server-livereload's src/index.js on line 99 like so:

app.use(config.proxies[i].source, proxy(proxyoptions));

When invoked in this way, without specifying a route via the options.route proxyOptions.route = '/api'; , the mount path is stripped out of req.url (which is caused by connect conventions to strictly enforce having trailing slashes).

// without options.route
req.url = /?query=param

// with options.route
req.url = ?query=param  

The effect of this is, when you have a route without a trailing slash, the path gets removed, but the mount point gets a slash added in by connect. So when the path p1 and p2 get put through slashJoin() a slash is added in and gives me the following incorrect path: (from the request object I was debugging):

path: '/search/?....'

On the destination server it is expecting /search, /search/ does not exist (these are different resources).

To fix this first part, we need to adjust the way we call into the proxy, by setting the route parameter on the options object in src/index.js instead

proxyoptions.route = config.proxies[i].source;
app.use(proxy(proxyoptions));

This will now give us a properly mounted route. The final step is to make sure we don't automatically add an unnecessary slash in via the slashJoin() function automatically. We should only add the slash in if p1 has a trailing slash, and p2 has a leading slash (basically we collapse it into one)

 function slashJoin(p1, p2) {
   if (p1.length && p1[p1.length - 1] === '/' && p2.length && p2[0] === '/') {p2 = p2.substring(1); }
   return p1 + p2;
}

This way, if there is no trailing slash, we don't add one. I was going to write a test up for this, but the test suite already has a test covering this use case.

✓ Can proxy just the given route with query.

If this made any sense and you feel comfortable merging my pending changes, I would really appreciate it. I'm going to go bug @hiddentao to alter the invocation of your module and hopefully be on my way to using your two great libraries.

TL;DR - slashJoin always adds a slash between p1 and p2, even if there is no trailing slash on the p1 path route, or leading slash on the p2 route. This is incorrect. I fixed this automatic URL rewrite, ensured tests pass, and ensured the overall use case works.

[hiddentao]

@jlank I've updated gulp-server-livereload to use the latest version of this library and have made the change you've suggested. Hope that helps.

[jlank]

@hiddentao that is great thank you, I'll test it out now!

[jlank]

@hiddentao works for me, which is great, thank you. @andrewrk the version at master works for my use case and passes the existing tests so we may be able to ignore this PR, because I think the logic is the same as before.