-
Notifications
You must be signed in to change notification settings - Fork 115
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
Exclude default ports 80/443 from base URL #69
base: master
Are you sure you want to change the base?
Conversation
fc9e62b
to
2886137
Compare
oauth-1.0a.js
Outdated
}, | ||
|
||
// Parse url into components. | ||
parseUrl: { |
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 defined the helper as a static method (easier for testing) but I am happy to define it on the prototype like the other helpers if you prefer.
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.
is static method is supported by older nodejs or browser?
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.
IE9+, Safari 5+ and evergreen browser support Object.defineProperties
. I am guessing every node versions support ES 5.1; node 0.10 certainly supports Object.defineProperties
.
If you need better support, I can define it as OAuth.parseUrl = function(url) {...}
.
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.
how about prototype
?
2886137
to
a755e72
Compare
too many changes. take me time to review all of them, be patient |
this PR only "Exclude default ports 80/443 from base URL"? why take 13 files changes |
b25563d
to
5b6701d
Compare
@ddo Rebased on master |
5b6701d
to
d302d34
Compare
d302d34
to
6c14755
Compare
Define |
That's crud fix. Maybe we should parse the URL without reg. exp. It would avoid any breaking changes. |
i still checking the regex part. any source? |
Based on https://en.wikipedia.org/wiki/URL (although I just notice the password part of the auth should be optional). The host is optional but required for OAuth request:
|
i think we do it too complicated for this simple task? |
Possibly. We don't need to extract the scheme + port and later the query with the same regexp. |
var match = this._urlPattern.exec(url); | ||
var components; | ||
|
||
if (match == null || match.len < 8) { |
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 really don't understand this line. does this check http
prefix?
Fix #59
Breaking changes: the URL is validated