-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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.
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
url: fixing url resolve issue #278
Conversation
@@ -663,7 +663,7 @@ Url.prototype.resolveObject = function(relative) { | |||
// then it must NOT get a trailing slash. | |||
var last = srcPath.slice(-1)[0]; | |||
var hasTrailingSlash = ( | |||
(result.host || relative.host) && (last === '.' || last === '..') || | |||
(result.host || relative.host || srcPath.length > 1) && (last === '.' || last === '..') || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: long lines should wrap at 80 columns. make jslint
will warn you about that.
Does |
@bnoordhuis Yes, it passes the tests. I've styled the code, thanks for mentioning it. |
@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. |
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) |
@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. |
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:
|
Makes sense - those are both pretty solid things to consider. |
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. |
@cjihrig Looks like the node PR was landed v0.10 by @trevnorris |
@cjihrig given this landed in node, is this actually semver-major? |
I would say no. Since it landed in 0.10, I would consider it a bug fix. |
'.' 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 <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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)
@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? |
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. |
It doesn't matter if it is a bug fix or not, it's backwards-incompatible.
Is io.js not following semver now? |
url.resolve('/path/to/file', '.')
should return/path/to/
with the trailing slash.Also
url.resolve('/', '.')
should return/
.