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.

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

Merged
merged 1 commit into from
Dec 2, 2014
Merged

Conversation

samccone
Copy link
Contributor

@samccone samccone commented Dec 1, 2014

No description provided.

@samccone
Copy link
Contributor Author

samccone commented Dec 1, 2014

Composite PR of

#744
#746

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.
@samccone samccone changed the title Update common.js [bugfix] handle multi ? urls Dec 1, 2014
lastSegs[1] && retSegs.push(lastSegs[1]);

// Handle case where there could be multiple ? in the URL.
retSegs.concat(lastSegs);
Copy link
Contributor

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.

Copy link
Contributor

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('?')

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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! ^-^!

@koolc koolc mentioned this pull request Dec 2, 2014
jcrugzz added a commit that referenced this pull request Dec 2, 2014
@jcrugzz jcrugzz merged commit 70ed1c4 into http-party:master Dec 2, 2014
@samccone
Copy link
Contributor Author

samccone commented Dec 3, 2014

ah sorry for the delay @jcrugzz I was traveling for the last 24 hours but it looks like this was resolved 👍

@samccone samccone deleted the sjs/fix-common branch December 3, 2014 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants