Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

No join method for URI paths #2216

Closed
OrangeDog opened this Issue Nov 28, 2011 · 24 comments

Comments

Projects
None yet

Now that path.join uses the platform separator, there is no function for safely constructing URIs. Either an additional argument to path.join, or a new function in the path or url modules?

celer commented Dec 28, 2012

Just having done a major port of a large node.js app to Windows this would be very helpful to have and was a major pain point.

url.joinPath or url.join should exist, and documentation in path.join should be updated to reflect that it should not be used to join url paths, but that url.join should be utilized instead.

McDeez commented Dec 28, 2012

Having this same issue writing multi-platform node.js code. url.join or path.webjoin would be really useful.

celer commented Dec 28, 2012

For example:

diff --git a/lib/url.js b/lib/url.js
index b8ba3fb..26044fd 100644
--- a/lib/url.js
+++ b/lib/url.js
@@ -27,6 +27,37 @@ exports.resolveObject = urlResolveObject;
 exports.format = urlFormat;

 exports.Url = Url;
+  
+// path.normalize(path)
+// this is a copy of the posix path.normalize in path 
+exports.normalize = function(path) {
+  var isAbsolute = path.charAt(0) === '/',
+      trailingSlash = path.substr(-1) === '/';
+
+  // Normalize the path
+  path = normalizeArray(path.split('/').filter(function(p) {
+    return !!p;
+  }), !isAbsolute).join('/');
+
+  if (!path && !isAbsolute) {
+    path = '.';
+  }
+  if (path && trailingSlash) {
+    path += '/';
+  }
+
+  return (isAbsolute ? '/' : '') + path;
+};
+
+
+// path.join(path)
+// this is a copy of the posix path.join in path 
+exports.join = function() {
+  var paths = Array.prototype.slice.call(arguments, 0);
+  return exports.normalize(paths.filter(function(p, index) {
+    return p && typeof p === 'string';
+  }).join('/'));
+};

 function Url() {
   this.protocol = null;

bmeck commented Jan 8, 2013

This is more complicated because urls have clearly defined parts that do not math with simple delineation:

require('path').join('x?y','z')
'x?y/z'

We would need to determine the semantics of joining the various url parts, query and hash in particular. Rest should probably just perform clobbering.

rlidwka commented Jan 9, 2013

Why would anybody want that?

URL path separator is /, and it's the same everywhere. Why don't just use string concatenation?

PS: /some//path/ is an invalid unix path, but it's a perfectly valid URL. And there's a lot of simular edge cases for URLs.

McDeez commented Jan 9, 2013

Rlidwka, you haven't worked on windows much then. Its really useful as many libraries use it for urls as well as files. But when using those in a cross platform app, you quickly realize the problem.

isaacs commented Jan 9, 2013

@mcdick What @rlidwka is saying is that, if you're joining a url path, you can just use parts.join('/') or start + '/' + end etc. It's only if you're joining file system paths that you need the path.join() or path.resolve() functionality, because you don't want to preserve empty bits and you need to use the system-specific delimiter.

Just don't use path.join for urls, and you're fine. There's already a url.resolve if you need to do fancy resolution, but url.join() is complicated and confusing to even know what it's supposed to do.

Let's not do this, and instead, just use string concatenation.

@isaacs isaacs closed this Jan 9, 2013

@ghost ghost referenced this issue in sintaxi/dbox May 2, 2013

Open

path.join #66

elgalu commented Feb 5, 2014

Hi @isaacs, coming from the Ruby world i'm frankly surprised you are happy with using string concatenation for joining URIs, forcibly us doing thinks like

[pathA.replace(/^\/|\/$/g,""),pathB.replace(/^\/|\/$/g,"")].join("/")

Will leave you here some nice tell don't ask DSL from the Ruby URI class that may be worth of inspiration:

require 'uri'

URI.join('http://example.com', 'foo').to_s
#=> "http://example.com/foo"

URI.join('http://example.com', '/foo', '/bar').to_s
#=> "http://example.com/bar"

URI.join('http://example.com', '/foo', 'bar').to_s
#=> "http://example.com/bar"

URI.join('http://example.com', '/foo/', 'bar').to_s
#=> "http://example.com/foo/bar"

URI.join('http://example.com/', '/example').to_s
#=> "http://example.com/example"

URI.join('http://example.com/example', 'test').to_s
#=> "http://example.com/test"

URI.join('http://example.com/example/', 'test').to_s
#=> "http://example.com/example/test"

URI.join('http://example.com/example/foo', '../css').to_s
#=> "http://example.com/css"

It would be awesome if a similar functionality as Ruby's URI.join function exists in node 👍

zdne commented Mar 3, 2014

👍

abtris commented Mar 3, 2014

👍

ecordell commented Mar 6, 2014

👍

Almad commented Mar 6, 2014

👍

Baggz commented Mar 6, 2014

👍

medikoo commented Mar 6, 2014

Environment agnostic, posix version of join:

var join = require('path2/posix/join');

See: https://github.com/medikoo/path2#path2

👍

@dougwilson dougwilson referenced this issue in mscdex/express-optimized Jul 7, 2014

Open

what does querystring.js do? #2

tamtakoe commented Mar 8, 2015

👍

gilbert commented May 27, 2015

👍

uhoreg commented May 28, 2015

@elgalu, @robinvdvleuten, other than not being able to take more than two arguments, it looks like Node's url.resolve does the same thing as Ruby's URI.join.

wearhere added a commit to mixmaxhq/meteor-electron that referenced this issue Dec 20, 2015

Fix URL concatenation
Some servers (*cough*Cloudfront*cough*) reject double-slashes, so blind
string concatenation (nodejs/node-v0.x-archive#2216 (comment))
is not adequate to combine URLs.

👍

ivan-kleshnin commented May 25, 2017 edited

Why would anybody want that?

Because

p1 + "/" + p2 

implies p1 never ends with "/". And URLs may very well end with it. So better logic for general case is:

p1.endsWith("/") ? p1 + p2 : p1 + "/" + p2

And then you may want to handle leading slashes for p2 as well.
So no, it's not so easy as "just use concatenation". Still I agree that semantics can be quite complicated once you start to consider query params, hashmarks and whatnot. Maybe this really should belong to userland.

ArmorDarks commented Jul 11, 2017 edited

@isaacs maybe that issue should be reviewed once again?

Argumentation for closing it is quite vague. Simple concatenation won't work well, since it isn't that simple for urls.

I understand that getting joining for url working is uneasy task, but, fortunately, there is a lot of work already been done in this field by other people.

For instance, URI.js and its joinPaths method, which intelligently resolves this issue.

Of course, it would be much more convenient to see similar method as part of Node core modules, since to achieve this functionality URI.js implements similar to good portion of Node's URL methods. which makes a bit regretful crossover.

lobabob commented Oct 11, 2017

👍

@bnoordhuis bnoordhuis locked and limited conversation to collaborators Oct 12, 2017

Member

bnoordhuis commented Oct 12, 2017

This repo has been archived. If you want to continue the conversation, please move it to nodejs/node.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.