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

Added an option to disable the URL rewrite when using proxy tables #188

Closed
wants to merge 1 commit into from
Closed

Conversation

alexandernilsson
Copy link

Ran into some problems when proxying HTTP to one place and Socket.IO traffic to another because of the URL rewrite, so added an option to disable it.

@ovaillancourt
Copy link

+1 on that, I'd definitely like to see this feature in. (had to hack in something similar 5 days ago)

Suggestions on your edits:

  1. I'd put the split from line 142 inside a first if that verifies the rewrite url option, then make the split, and then make a 2nd if that checks if the length is > 1 to prevent doing the split on every request (as the result from the split will never be used anyway if the rewriteUrl option is set to false).
  2. I'd completely invert the option. For example, make it keepUrlSegments which does not rewrite if set to true. This way you keep the current default behavior (to rewrite everytime) the same if you omit the option, which preserves backward compatibility. Otherwise, everyone who's currently using the default behavior will have to go add a rewriteUrl: true to their code.
  3. Write tests for the new cases, I don't think it's gonna be pulled in otherwise.

@alexandernilsson
Copy link
Author

  1. That's a good idea.
  2. rewriteUrl defaults to true, so it wouldn't break backwards compatibility. But actually, I do kind off agree with you that keepUrlSegments sounds better.
  3. True, will have a look at that.

@ovaillancourt
Copy link

Oops you're right for # 2! Didn't look close enough to your default. Sorry about that :D.

@indexzero
Copy link
Contributor

This implementation has changed significantly since this was originally submitted. Still open to the feature idea, but you need to pull and re-implement.

@indexzero indexzero closed this Jul 22, 2012
@ovaillancourt
Copy link

Quick note:

This issue has been resolved by commit 55286a7 - doesn't need to be pulled in anymore.

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