[Pkg-javascript-commits] [node-stream-http] 86/208: Drastically simplify and improve url handling

[Bastien Roucariès]

This is an automated email from the git hooks/post-receive script.

rouca pushed a commit to branch master
in repository node-stream-http.

commit 6b9d3d107cba80263d99ea323f593e982641741f
Author: John Hiesey <john at hiesey.com>
Date:   Thu Sep 10 22:29:44 2015 -0700

    Drastically simplify and improve url handling
    
    Make options handling logic match node's `http` module more
    closely and let the browser handle all of the relative url
    cases itself.
    
    This has a few important advantages:
    * Behavior is closer to native `http` modula
    * Relative paths in urls are handled properly (fixes #16)
    * Code is simpler and more obviously correct
---
 index.js                     | 22 +++++++++++-----------
 lib/request.js               |  6 ++----
 test/node/http-browserify.js | 40 +++++++++++++++++++++-------------------
 3 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/index.js b/index.js
index 3cd3ff4..eedc173 100644
--- a/index.js
+++ b/index.js
@@ -11,19 +11,19 @@ http.request = function (opts, cb) {
 	else
 		opts = extend(opts)
 
-	// Split opts.host into its components
-	var hostHostname = opts.host ? opts.host.split(':')[0] : null
-	var hostPort = opts.host ? parseInt(opts.host.split(':')[1], 10) : null
+	var protocol = opts.protocol || ''
+	var host = opts.hostname || opts.host
+	var port = opts.port
+	var path = opts.path || '/'
 
-	opts.method = opts.method || 'GET'
+	// Necessary for IPv6 addresses
+	if (host && host.indexOf(':') !== -1)
+		host = '[' + host + ']'
+
+	// This may be a relative url. The browser should always be able to interpret it correctly.
+	opts.url = (host ? (protocol + '//' + host) : '') + (port ? ':' + port : '') + path
+	opts.method = (opts.method || 'GET').toUpperCase()
 	opts.headers = opts.headers || {}
-	opts.path = opts.path || '/'
-	opts.protocol = opts.protocol || window.location.protocol
-	// If the hostname is provided, use the default port for the protocol. If
-	// the url is instead relative, use window.location.port
-	var defaultPort = (opts.hostname || hostHostname) ? (opts.protocol === 'https:' ? 443 : 80) : window.location.port
-	opts.hostname = opts.hostname || hostHostname || window.location.hostname
-	opts.port = opts.port || hostPort || defaultPort
 
 	// Also valid opts.auth, opts.mode
 
diff --git a/lib/request.js b/lib/request.js
index cb7d012..b952a6a 100644
--- a/lib/request.js
+++ b/lib/request.js
@@ -31,8 +31,6 @@ var ClientRequest = module.exports = function (opts) {
 	stream.Writable.call(self)
 
 	self._opts = opts
-	var hostname = opts.hostname.indexOf(':') === -1 ? opts.hostname : '[' + opts.hostname + ']'
-	self._url = opts.protocol + '//' + hostname + ':' + opts.port + opts.path
 	self._body = []
 	self._headers = {}
 	if (opts.auth)
@@ -116,7 +114,7 @@ ClientRequest.prototype._onFinish = function () {
 			return [headersObj[name].name, headersObj[name].value]
 		})
 
-		window.fetch(self._url, {
+		window.fetch(self._opts.url, {
 			method: self._opts.method,
 			headers: headers,
 			body: body,
@@ -131,7 +129,7 @@ ClientRequest.prototype._onFinish = function () {
 	} else {
 		var xhr = self._xhr = new window.XMLHttpRequest()
 		try {
-			xhr.open(self._opts.method, self._url, true)
+			xhr.open(self._opts.method, self._opts.url, true)
 		} catch (err) {
 			process.nextTick(function () {
 				self.emit('error', err)
diff --git a/test/node/http-browserify.js b/test/node/http-browserify.js
index 144fbca..7f75935 100644
--- a/test/node/http-browserify.js
+++ b/test/node/http-browserify.js
@@ -1,15 +1,12 @@
 // These tests are teken from http-browserify to ensure compatibility with
 // that module
 var test = require('tape')
+var url = require('url')
 
-global.window = {}
-window.location = {
-		hostname: 'localhost',
-		port: 8081,
-		protocol: 'http:'
-}
+var location = 'http://localhost:8081'
 
 var noop = function() {}
+global.window = {}
 window.XMLHttpRequest = function() {
 	this.open = noop
 	this.send = noop
@@ -21,16 +18,17 @@ delete require.cache[moduleName]
 var http = require('../../')
 
 test('Test simple url string', function(t) {
-	var url = { path: '/api/foo' }
-	var request = http.get(url, noop)
+	var testUrl = { path: '/api/foo' }
+	var request = http.get(testUrl, noop)
 
-	t.equal(request._url, 'http://localhost:8081/api/foo', 'Url should be correct')
+	var resolved = url.resolve(location, request._opts.url)
+	t.equal(resolved, 'http://localhost:8081/api/foo', 'Url should be correct')
 	t.end()
 
 })
 
 test('Test full url object', function(t) {
-	var url = {
+	var testUrl = {
 		host: "localhost:8081",
 		hostname: "localhost",
 		href: "http://localhost:8081/api/foo?bar=baz",
@@ -44,9 +42,10 @@ test('Test full url object', function(t) {
 		slashes: true
 	}
 
-	var request = http.get(url, noop)
+	var request = http.get(testUrl, noop)
 
-	t.equal(request._url, 'http://localhost:8081/api/foo?bar=baz', 'Url should be correct')
+	var resolved = url.resolve(location, request._opts.url)
+	t.equal(resolved, 'http://localhost:8081/api/foo?bar=baz', 'Url should be correct')
 	t.end()
 })
 
@@ -60,15 +59,17 @@ test('Test alt protocol', function(t) {
 
 	var request = http.get(params, noop)
 
-	t.equal(request._url, 'foo://localhost:3000/bar', 'Url should be correct')
+	var resolved = url.resolve(location, request._opts.url)
+	t.equal(resolved, 'foo://localhost:3000/bar', 'Url should be correct')
 	t.end()
 })
 
 test('Test string as parameters', function(t) {
-	var url = '/api/foo'
-	var request = http.get(url, noop)
+	var testUrl = '/api/foo'
+	var request = http.get(testUrl, noop)
 
-	t.equal(request._url, 'http://localhost:8081/api/foo', 'Url should be correct')
+	var resolved = url.resolve(location, request._opts.url)
+	t.equal(resolved, 'http://localhost:8081/api/foo', 'Url should be correct')
 	t.end()
 })
 
@@ -88,10 +89,11 @@ test('Test withCredentials param', function(t) {
 })
 
 test('Test ipv6 address', function(t) {
-	var url = 'http://[::1]:80/foo'
-	var request = http.get(url, noop)
+	var testUrl = 'http://[::1]:80/foo'
+	var request = http.get(testUrl, noop)
 
-	t.equal(request._url, 'http://[::1]:80/foo', 'Url should be correct')
+	var resolved = url.resolve(location, request._opts.url)
+	t.equal(resolved, 'http://[::1]:80/foo', 'Url should be correct')
 	t.end()
 })
 

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-javascript/node-stream-http.git