url: fixing url resolve issue#278
Uh oh!
There was an error while loading. Please reload this page.
Conversation
lib/url.js Outdated
There was a problem hiding this comment.
Style: long lines should wrap at 80 columns. make jslint will warn you about that.
bnoordhuis commented Jan 10, 2015
Does |
amir-s commented Jan 11, 2015
@bnoordhuis Yes, it passes the tests. I've styled the code, thanks for mentioning it. |
bnoordhuis commented Jan 11, 2015
@amir-s Thanks. One more thing, can you squash the commits and reword the commit log to something that conforms to the contributing guide? (Check Can someone who uses the url module on a daily basis chime in? This PR changes the result of e.g. |
ljharb commented Jan 11, 2015
Would it be out of the question to make (I note that https://www.npmjs.com/package/url already exists, but not sure if that changes anything) |
amir-s commented Jan 11, 2015
@bnoordhuis@ljharb About the url package that already exists in npm, it is just a copy of node's url module for using it with Browserify. |
chrisdickinson commented Jan 11, 2015
Chiming in here: the goal for the url module is to move towards WHATWG compliance. The dot behavior here seems to be in line with the intent of that spec as I understand it. +1.
Two things to consider:
|
ljharb commented Jan 12, 2015
Makes sense - those are both pretty solid things to consider. |
cjihrig commented Jan 12, 2015
My vote is to let the PR on joyent/node run its course. If it gets merged then io.js will get it eventually. If it is rejected, we can reevaluate. I'm +1 on increasing WHATWG compliance. EDIT: Of course, then we also run the risk of the joyent/node PR sitting indefinitely. |
trevnorris added the url 
brendanashworth added the semver-major 
Fishrock123 commented Feb 10, 2015
@cjihrig Looks like the node PR was landed v0.10 by @trevnorris |
Fishrock123 commented Feb 13, 2015
@cjihrig given this landed in node, is this actually semver-major? |
cjihrig commented Feb 13, 2015
I would say no. Since it landed in 0.10, I would consider it a bug fix. |
Fishrock123 removed the semver-major 
'.' and '..' are directory specs and resolving urls with or without the hostname with '.' and '..' should add a trailing slash to the end of the url. Fixes: nodejs/node-v0.x-archive#8992 PR-URL: #278 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
cjihrig commented Feb 13, 2015
Thanks. Landed in faa687b |
Notable changes: * url: `url.resolve('/path/to/file', '.')` now returns `/path/to/` with the trailing slash, `url.resolve('/', '.')` returns `/`. #278 (Amir Saboury) * tls: tls (and in turn https) now rely on a stronger default cipher suite which excludes the RC4 cipher. If you still want to use RC4, you have to specify your own ciphers suite. #826 (Roman Reiss)
thekidfrankensteinthrewinapond commented Feb 20, 2015
@cjihrig This is a breaking change, here's the documentation: https://iojs.org/api/url.html#url_url_resolve_from_to Why is this not in 2.0? |
cjihrig commented Feb 20, 2015
I would classify this as a bug fix. Unfortunately, bug fixes sometimes change behavior. The fact that it went into the 0.10 (stable) branch of Node.js further convinced me that it didn't warrant a major version bump. I apologize if this caused you any problems. |
thekidfrankensteinthrewinapond commented Feb 20, 2015
It doesn't matter if it is a bug fix or not, it's backwards-incompatible. Is io.js not following semver now? |
11 participants
url.resolve('/path/to/file', '.')should return/path/to/with the trailing slash.Also
url.resolve('/', '.')should return/.