-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[bugfix] handle multi ? urls #748
Conversation
4a6c8e2
to
8581910
Compare
Without this fix urls that had multiple ? in them would drop sections of the url since before there was an assumption of there only being one.
8581910
to
70ed1c4
Compare
lastSegs[1] && retSegs.push(lastSegs[1]); | ||
|
||
// Handle case where there could be multiple ? in the URL. | ||
retSegs.concat(lastSegs); |
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.
use retSegs.push.apply(retSegs, lastSegs);
as that modifies the retSegs
array. Concat returns a new array and we want to prevent new array creation. This should also fix the test from failing.
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.
Do you confirm the code is correct? o?o
I think, the code retSegs.concat(lastSegs)
doesn't seemingly change the array retSegs
value when it doesn't assigned to retSegs
like retSegs = retSegs.concat(lastSegs);
, so wouldn't the retSegs
value in the last statement return retSegs.join('?')
be original? You can try by a multiple '?' urls.
The codes recommended:
retSegs.push.apply(retSegs, lastSegs);
return retSegs.join('?');
or
return retSegs.concat(lastSegs).join('?')
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.
retSegs.push.apply(retSegs, lastSegs);
return retSegs.join('?');
because it doesnt create a new array
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 use 'concat' can create a new array indeed, but why to need a new array? And that, the new array isn't used by you to the codes after it. So, the variable retSegs
value in return statement is still the old value, that is to say, the array length of the variable retSegs
is still 1.
If you want to use 'concat' function, an assignment statement is required, like retSegs = retSegs.concat(lastSegs)
, because of the example below.
var a = [1, 2];
var b = a.concat([3,4]); // to assign the variable 'b'.
console.log(a, b)
// output: [1, 2] [1, 2, 3, 4]
var c = a.join('-'); // ! This is your case: 'a' doesn't been changed and is still old value.
var d = b.join('-'); // ! This is my meaning: new elements have been added into 'b'.
console.log(c, d);
// output: 1-2 1-2-3-4
/* expected result: 1-2-3-4, instead of: 1-2 */
So, I think that the code retSegs.concat(lastSegs)
should be changed to retSegs = retSegs.concat(lastSegs)
. Do you?
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.
I understand there is a bit of a language barrier here but the code should be...
retSegs.push.apply(retSegs, lastSegs);
return retSegs.join('?');
We DO NOT want to create a new array. I understand how concat works which is why I do not want to use 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.
OK, I see now, and the codes in master are already right.
I just had doubts when I saw the codes in the branch http-proxy/common.js#L160 from #748 are below:
// Handle case where there could be multiple ? in the URL.
retSegs.concat(lastSegs);
return retSegs.join('?')
So sorry! ^-^!
ah sorry for the delay @jcrugzz I was traveling for the last 24 hours but it looks like this was resolved 👍 |
No description provided.