Skip to content

Commit

Permalink
Revert "url: fix treatment of some values as non-empty"
Browse files Browse the repository at this point in the history
This reverts commit 6687721.

It was agreed that this change contained too much potential ecosystem
breakage, particularly around the inability to `delete` properties off a
`Url` object. It may be re-introduced for a later release, along with
better work on ecosystem compatibility.

PR-URL: #1602
Reviewed-By: Mikeal Rogers <mikeal.rogers@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Forrest L Norvell <forrest@npmjs.com>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Isaac Z. Schlueter <i@izs.me>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
  • Loading branch information
rvagg committed May 4, 2015
1 parent 6687721 commit 0f39ef4
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 61 deletions.
24 changes: 10 additions & 14 deletions lib/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -879,7 +879,7 @@ Object.defineProperty(Url.prototype, 'port', {
return null;
},
set: function(v) {
if (isConsideredEmpty(v)) {
if (v === null) {
this._port = -1;
if (this._host)
this._host = null;
Expand Down Expand Up @@ -941,7 +941,7 @@ Object.defineProperty(Url.prototype, 'path', {
return (p == null && s) ? ('/' + s) : null;
},
set: function(v) {
if (isConsideredEmpty(v)) {
if (v === null) {
this._pathname = this._search = null;
return;
}
Expand Down Expand Up @@ -973,7 +973,7 @@ Object.defineProperty(Url.prototype, 'protocol', {
return proto;
},
set: function(v) {
if (isConsideredEmpty(v)) {
if (v === null) {
this._protocol = null;
} else {
var proto = '' + v;
Expand Down Expand Up @@ -1001,7 +1001,7 @@ Object.defineProperty(Url.prototype, 'href', {
var parsesQueryStrings = this._parsesQueryStrings;
// Reset properties.
Url.call(this);
if (!isConsideredEmpty(v))
if (v !== null && v !== '')
this.parse('' + v, parsesQueryStrings, false);
},
enumerable: true,
Expand All @@ -1013,7 +1013,7 @@ Object.defineProperty(Url.prototype, 'auth', {
return this._auth;
},
set: function(v) {
this._auth = isConsideredEmpty(v) ? null : '' + v;
this._auth = v === null ? null : '' + v;
this._href = '';
},
enumerable: true,
Expand All @@ -1026,7 +1026,7 @@ Object.defineProperty(Url.prototype, 'host', {
return this._host;
},
set: function(v) {
if (isConsideredEmpty(v)) {
if (v === null) {
this._port = -1;
this._hostname = this._host = null;
} else {
Expand All @@ -1053,7 +1053,7 @@ Object.defineProperty(Url.prototype, 'hostname', {
return this._hostname;
},
set: function(v) {
if (isConsideredEmpty(v)) {
if (v === null) {
this._hostname = null;

if (this._hasValidPort())
Expand All @@ -1080,7 +1080,7 @@ Object.defineProperty(Url.prototype, 'hash', {
return this._hash;
},
set: function(v) {
if (isConsideredEmpty(v) || v === '#') {
if (v === null) {
this._hash = null;
} else {
var hash = '' + v;
Expand All @@ -1100,7 +1100,7 @@ Object.defineProperty(Url.prototype, 'search', {
return this._search;
},
set: function(v) {
if (isConsideredEmpty(v) || v === '?') {
if (v === null) {
this._search = this._query = null;
} else {
var search = escapeSearch('' + v);
Expand All @@ -1125,7 +1125,7 @@ Object.defineProperty(Url.prototype, 'pathname', {
return this._pathname;
},
set: function(v) {
if (isConsideredEmpty(v)) {
if (v === null) {
this._pathname = null;
} else {
var pathname = escapePathName('' + v).replace(/\\/g, '/');
Expand All @@ -1144,10 +1144,6 @@ Object.defineProperty(Url.prototype, 'pathname', {
configurable: true
});

function isConsideredEmpty(value) {
return value === null || value === undefined || value === '';
}

// Search `char1` (integer code for a character) in `string`
// starting from `fromIndex` and ending at `string.length - 1`
// or when a stop character is found.
Expand Down
59 changes: 12 additions & 47 deletions test/parallel/test-url-accessors.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ const accessorTests = [{
}, {
// Setting href to non-null non-string coerces to string
url: 'google',
set: {href: 0},
set: {href: undefined},
test: {
path: '0',
href: '0'
path: 'undefined',
href: 'undefined'
}
}, {
// Setting port is reflected in host
Expand Down Expand Up @@ -180,8 +180,8 @@ const accessorTests = [{
url: 'http://www.google.com',
set: {search: ''},
test: {
search: null,
path: '/'
search: '?',
path: '/?'
}
}, {

Expand All @@ -203,11 +203,10 @@ const accessorTests = [{
}, {

// Empty hash is ok
url: 'http://www.google.com#hash',
url: 'http://www.google.com',
set: {hash: ''},
test: {
hash: null,
href: 'http://www.google.com/'
hash: '#'
}
}, {

Expand Down Expand Up @@ -253,8 +252,7 @@ const accessorTests = [{
url: 'http://www.google.com',
set: {pathname: ''},
test: {
pathname: null,
href: 'http://www.google.com'
pathname: '/'
}
}, {
// Null path is ok
Expand Down Expand Up @@ -292,12 +290,11 @@ const accessorTests = [{
protocol: null
}
}, {
// Empty protocol is ok
// Empty protocol is invalid
url: 'http://www.google.com/path',
set: {protocol: ''},
test: {
protocol: null,
href: '//www.google.com/path'
protocol: 'http:'
}
}, {
// Set query to an object
Expand Down Expand Up @@ -330,9 +327,9 @@ const accessorTests = [{
url: 'http://www.google.com/path?key=value',
set: {path: '?key2=value2'},
test: {
pathname: null,
pathname: '/',
search: '?key2=value2',
href: 'http://www.google.com?key2=value2'
href: 'http://www.google.com/?key2=value2'
}
}, {
// path is reflected in search and pathname 3
Expand All @@ -352,38 +349,6 @@ const accessorTests = [{
search: null,
href: 'http://www.google.com'
}
}, {
// setting hash to '' removes any hash
url: 'http://www.google.com/#hash',
set: {hash: ''},
test: {
hash: null,
href: 'http://www.google.com/'
}
}, {
// setting hash to '#' removes any hash
url: 'http://www.google.com/#hash',
set: {hash: '#'},
test: {
hash: null,
href: 'http://www.google.com/'
}
}, {
// setting search to '' removes any search
url: 'http://www.google.com/?search',
set: {search: ''},
test: {
search: null,
href: 'http://www.google.com/'
}
}, {
// setting search to '?' removes any search
url: 'http://www.google.com/?search',
set: {search: '?'},
test: {
search: null,
href: 'http://www.google.com/'
}
}

];
Expand Down

0 comments on commit 0f39ef4

Please sign in to comment.