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

Use the auth property of the parsed URL #239

Merged
merged 1 commit into from
May 5, 2015

Conversation

lpinca
Copy link
Contributor

@lpinca lpinca commented May 5, 2015

There is no need to set the auth property in the parsed URL, becuase the url-parse module parses it correctly.

@brycekahle
Copy link
Contributor

Great. Thanks for adding the tests too.

brycekahle added a commit that referenced this pull request May 5, 2015
Use the `auth` property of the parsed URL
@brycekahle brycekahle merged commit 3a93e2e into sockjs:master May 5, 2015
@lpinca lpinca deleted the use/url.auth branch May 5, 2015 18:50
@3rd-Eden
Copy link

3rd-Eden commented May 5, 2015

The code can be simplified even further. If the parsedUrl.set('path', replaceshizzle) is used instead of the replace logic you can just use parsedUrl.href for this.url as all that logic is also done in the url-parse module

@brycekahle
Copy link
Contributor

Cool. I'll try and work that in. Thanks @3rd-Eden

@lpinca
Copy link
Contributor Author

lpinca commented May 5, 2015

@3rd-Eden doesn't that also include the hash which is stripped away right now? I guess the same parsedUrl.set('hash', ''); can be used against that.

@3rd-Eden
Copy link

3rd-Eden commented May 5, 2015

It doesnt need stripping as its already throwing an error when a hash is included a few lines above this.

Arnout Kazemier

On May 5, 2015, at 9:49 PM, Luigi Pinca notifications@github.com wrote:

@3rd-Eden doesn't that also include the hash which is stripped away right now? I guess the same parsedUrl.set('hash', ''); can be used against that.


Reply to this email directly or view it on GitHub.

@lpinca
Copy link
Contributor Author

lpinca commented May 5, 2015

You are right, I've missed that.

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