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.

Sign up for GitHub

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

Jump to bottom

url: return valid file: urls fom url.format() #7234

Closed
wants to merge 3 commits into from
Closed

url: return valid file: urls fom url.format() #7234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e8c661
url: return valid file: urls fom url.format()
Trott Jun 8, 2016
99f87da
squash: use string concatenation instead of template
Trott Jun 9, 2016
4c9f18c
squash: startsWith() -> charCodeAt()
Trott Jun 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions lib/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ Url.prototype.format = function() {
var protocol = this.protocol || '';
var pathname = this.pathname || '';
var hash = this.hash || '';
var host = false;
var host = '';
var query = '';

if (this.host) {
Expand Down Expand Up @@ -602,13 +602,18 @@ Url.prototype.format = function() {

// only the slashedProtocols get the //. Not mailto:, xmpp:, etc.
// unless they had them to begin with.
if (this.slashes ||
(!protocol || slashedProtocol[protocol]) && host !== false) {
host = '//' + (host || '');
if (pathname && pathname.charCodeAt(0) !== 47/*/*/)
pathname = '/' + pathname;
} else if (!host) {
host = '';
if (this.slashes || slashedProtocol[protocol]) {
if (this.slashes || host) {
if (pathname && pathname.charCodeAt(0) !== 47/*/*/)
pathname = '/' + pathname;
host = '//' + host;
} else if (protocol.length >= 4 &&
protocol.charCodeAt(0) === 102/*f*/ &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: why not 0x.. instead?

Copy link
Member Author

@Trott Trott Jun 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny I used decimal because:

  • It's decimal in most of the existing charCodeAt() calls, including ~10 lines later in lines 621 and 622, as well as the switch statement ~30 lines earlier starting on line 581.
  • The sample code (provided by @mscdex) used decimal and I did a lazy copy/paste.

I don't feel strongly about it, though, and I'm happy to change it to hex format if that's the difference between a quick LGTM and a not-sure-about-this.

protocol.charCodeAt(1) === 105/*i*/ &&
protocol.charCodeAt(2) === 108/*l*/ &&
protocol.charCodeAt(3) === 101/*e*/) {
host = '//';
}
}

search = search.replace('#', '%23');
Expand Down
49 changes: 35 additions & 14 deletions test/parallel/test-url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
/* eslint-disable max-len */
'use strict';
require('../common');
var assert = require('assert');
const assert = require('assert');
const inspect = require('util').inspect;

var url = require('url');
const url = require('url');

// URLs to parse, and expected data
// { url : parsed }
Expand Down Expand Up @@ -881,8 +882,16 @@ for (const u in parseTests) {
}
});

assert.deepStrictEqual(actual, expected);
assert.deepStrictEqual(spaced, expected);
assert.deepStrictEqual(
actual,
expected,
`expected ${inspect(expected)}, got ${inspect(actual)}`
);
assert.deepStrictEqual(
spaced,
expected,
`expected ${inspect(expected)}, got ${inspect(spaced)}`
);

expected = parseTests[u].href;
actual = url.format(parseTests[u]);
Expand Down Expand Up @@ -1165,20 +1174,28 @@ var formatTests = {
hash: '#frag',
search: '?abc=the#1?&foo=bar',
pathname: '/fooA100%mBr',
},

// https://github.com/nodejs/node/issues/3361
'file:///home/user': {
href: 'file:///home/user',
protocol: 'file',
pathname: '/home/user',
path: '/home/user'
}
};
for (const u in formatTests) {
const expect = formatTests[u].href;
delete formatTests[u].href;
const actual = url.format(u);
const actualObj = url.format(formatTests[u]);
assert.equal(actual, expect,
'wonky format(' + u + ') == ' + expect +
'\nactual:' + actual);
assert.equal(actualObj, expect,
'wonky format(' + JSON.stringify(formatTests[u]) +
') == ' + expect +
'\nactual: ' + actualObj);
assert.strictEqual(actual, expect,
'wonky format(' + u + ') == ' + expect +
'\nactual:' + actual);
assert.strictEqual(actualObj, expect,
'wonky format(' + JSON.stringify(formatTests[u]) +
') == ' + expect +
'\nactual: ' + actualObj);
}

/*
Expand Down Expand Up @@ -1556,7 +1573,7 @@ var relativeTests2 = [
];
relativeTests2.forEach(function(relativeTest) {
const a = url.resolve(relativeTest[1], relativeTest[0]);
const e = relativeTest[2];
const e = url.format(relativeTest[2]);
assert.equal(a, e,
'resolve(' + [relativeTest[1], relativeTest[0]] + ') == ' + e +
'\n actual=' + a);
Expand Down Expand Up @@ -1598,9 +1615,13 @@ relativeTests2.forEach(function(relativeTest) {
var actual = url.resolveObject(url.parse(relativeTest[1]), relativeTest[0]);
var expected = url.parse(relativeTest[2]);

assert.deepStrictEqual(actual, expected);
assert.deepStrictEqual(
actual,
expected,
`expected ${inspect(expected)} but got ${inspect(actual)}`
);

expected = relativeTest[2];
expected = url.format(relativeTest[2]);
actual = url.format(actual);

assert.equal(actual, expected,
Expand Down