Update common.js

[koolc]

Bugfix: outgoing url is wrong when the source url is 'http://a.b.c/d/??e/a.js,f/b.js,g/c.js?t=123456' (a combo url).

[koolc]

When you merged the changed codes and update to a new version, please leave a message! Thank you!

[samccone]

Is this even a valid url?

[koolc]

This is a online url 'http://g.tbcdn.cn/??kissy/k/1.4.0/seed-min.js,kissy/k/1.4.0/base-min.js,kissy/k/1.4.0/json-min.js,kissy/k/1.4.0/dom/base-min.js,kissy/k/1.4.0/event/base-min.js,kissy/k/1.4.0/event/custom-min.js,kissy/k/1.4.0/event/dom/base-min.js,kissy/k/1.4.0/event/dom/focusin-min.js' on the Alibaba CDNs. You can take it
for test.

[jcrugzz]

@koolc is there a failing test this is compensating for? To be fair this logic is still not 100% correct but this change doesn't look like the right fix.

[koolc]

@jcrugzz @samccone
This is a online url 'http://g.tbcdn.cn/??kissy/k/1.4.0/seed-min.js,kissy/k/1.4.0/base-min.js,kissy/k/1.4.0/json-min.js,kissy/k/1.4.0/dom/base-min.js,kissy/k/1.4.0/event/base-min.js,kissy/k/1.4.0/event/custom-min.js,kissy/k/1.4.0/event/dom/base-min.js,kissy/k/1.4.0/event/dom/focusin-min.js' on the Alibaba CDNs. You can take it
for test.

This is a combo url(This is a technical measures that multiple requests will be merged into one in order to reduce the number of requests), and '??' is a combo tag.

I mainly think of this case, so maybe the changed logic is still not 100% correct, hope you can revise it.

[jcrugzz]

@koolc could you write a test case using this type of URL? I will look closer at the logic

[koolc]

A bin test file content:

#!/usr/bin/env node --harmony

/* ==========================================================================
   A Test Case for the method `common.urlJoin` of 'lib/http-proxy/common.js'
                                              - added by koolc on 2014.11.17
 ============================================================================
   - conditions:
     - the ingoing url is a combo url, such as 'http://g.tbcdn.cn/??kissy/
       k/1.4.0/seed-min.js,kissy/k/1.4.0/base-min.js,kissy/k/1.4.0/json-min
       .js,kissy/k/1.4.0/dom/base-min.js,kissy/k/1.4.0/event/base-min.js,
       kissy/k/1.4.0/event/custom-min.js,kissy/k/1.4.0/event/dom/base-min
       .js,kissy/k/1.4.0/event/dom/focusin-min.js'.

     - the host bind is 'g.tbcdn.cn 127.0.0.1'.

   - expectation:
     - the outgoing url is a whole url like origin url, except for the port,
       such as 'http://127.0.0.1:9000/??kissy/k/1.4.0/seed-min.js...'
 ========================================================================== */

var http = require('http');
var httpProxy = require('http-proxy');
var koa = require('koa');

// The proxy server
var proxy = httpProxy.createProxyServer();
http
    .createServer(function (req, res) {
        console.log('[proxy server] the path of the ingoing url: "' + req.url + '"');
        proxy.web(req, res, {
            target: {
                host: '127.0.0.1',
                port: 9000
            }
        }, function (err, req, res) {
            res.writeHead(500, {
                'Content-Type': 'text/plain'
            });
            res.end(err.message);
        });
    })
    .listen(80);

console.log('Proxy server started at ' + new Date());
console.log('Listening on http://127.0.0.1:80');
console.log('---------------------------------------------------------------');

// The web server
var app = koa();
app.use(function*(next){ // to validate the url value.
    // output path expected is "/??kissy/k/1.4.0/seed-min.js...focusin-min.js"
    console.log('[web   server] the path of the outgoing url: "' + this.url + '"'); // real output path: "/"
    yield next;
});

http.createServer(app.callback()).listen(9000);

console.log('Web server started at ' + new Date());
console.log('Listening on http://127.0.0.1:9000');
console.log('---------------------------------------------------------------');


console.log('The logs will output here. <Press Ctrl-C to quit>');

[jcrugzz]

@koolc In terms of a test case, could you add a test case to test of the common.setupOutgoing function in here

[koolc]

I think the test case(exists many '?') below in the common.setupOutgoing function could enough test out the problem. The codes:

    it('should not modify the query string and hash string', function () {
      var outgoing = {};
      common.setupOutgoing(outgoing, {
        target: { path: '/forward' },
      }, { url: '/?foo=bar//&target=http://foobar.com/?a=1%26b=2&other=some#id12' });

      expect(outgoing.path).to.eql('/forward/?foo=bar//&target=http://foobar.com/?a=1%26b=2&other=some#id12');
    })

I have sent a pull request , you can see my changes.

[koolc]

Maybe this below is better.

retSegs.push.apply(retSegs, lastSegs);

[jcrugzz]

Yea that change would be ideal, after that, this LGTM.

[koolc]

Excuse me, but I want to know when to support the change? My component will recently publish and it requires the 'http-proxy' module. So I expect the fresh version very much. Thank you!

[jcrugzz]

@koolc sorry for the late reply, once that change you already suggested is done, this should be merged. If you have anymore URL test cases for this function, it would be awesome to get more tests around this.

[koolc]

Sorry, I didn't fully understand what you mean, but the previous test case modified by me has been provided at #746 last week, and I'm sorry I have the only one application scene to use the function, so it's difficult for me to get more tests around this. Though, even if no more test cases, I'm sure that the function must be wrong. So, at least so far, I think it's the most important to correct it and the test cases can be accumulated slowly. Don't you think so?

[jcrugzz]

@koolc test cases are not required, I was just curious if you had anymore crazy URLs that could be sueful for testing ;). Just make the change for retSegs.push.apply(retSegs, lastSegs); and I will merge.

[koolc]

Thank you! I have seen the merged codes, but they doesn't seem to be completely right. Please see the message left by me on #748

[jcrugzz]

@koolc replied