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.

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

Closed
wants to merge 1 commit into from
Closed

url: fixing url resolve issue #278

wants to merge 1 commit into from

Conversation

amir-s
Copy link
Contributor

@amir-s amir-s commented Jan 10, 2015

url.resolve('/path/to/file', '.') should return /path/to/ with the trailing slash.
Also url.resolve('/', '.') should return /.

@@ -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 === '..') ||
Copy link
Member

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.

@bnoordhuis
Copy link
Member

Does make test pass with this change? And can you add a few new tests to test/parallel/test-url.js? Thanks.

@amir-s
Copy link
Contributor Author

amir-s commented Jan 11, 2015

@bnoordhuis Yes, it passes the tests. I've styled the code, thanks for mentioning it.
I also added some tests.
Thanks ;)

@bnoordhuis
Copy link
Member

@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 git log lib/url.js if you want some examples.)

Can someone who uses the url module on a daily basis chime in? This PR changes the result of e.g. url.resolve('/a/b/', '..') from '/a' to '/a/'. I'm unclear if that is the desired behavior. If nothing else, it diverges from joyent/node.

@ljharb
Copy link
Member

ljharb commented Jan 11, 2015

Would it be out of the question to make url be an independent module? That way, iojs shipping this as a breaking change wouldn't have to break code that also ran on node, since any project could just add url as a dependency.

(I note that https://www.npmjs.com/package/url already exists, but not sure if that changes anything)

@amir-s
Copy link
Contributor Author

amir-s commented Jan 11, 2015

@bnoordhuis @ljharb
As . and .. are directory specs, the returned string should be a directory, so it should end with a slash.
I just created a pull request in node's repo to address the issue, we can wait for them to merge it (nodejs/node-v0.x-archive#9010)

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
Copy link
Contributor

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.

@ljharb:

Would it be out of the question to make url be an independent module? That way, iojs shipping this as a breaking change wouldn't have to break code that also ran on node, since any project could just add url as a dependency.

Two things to consider:

  1. The package would have to be named something other than url, since core modules take precedence over local packages.
  2. Diverging iojs' url will diverge other APIs that depend on it -- namely, tls, http, and https. A user will not avoid incompatibility by explicitly depending on a userland version of an API, since the API may be used to implement other core APIs.

@ljharb
Copy link
Member

ljharb commented Jan 12, 2015

Makes sense - those are both pretty solid things to consider.

@cjihrig
Copy link
Contributor

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.

@amir-s amir-s changed the title fixing url resolve issue url: fixing url resolve issue Jan 14, 2015
@trevnorris trevnorris added the url Issues and PRs related to the legacy built-in url module. label Jan 22, 2015
@brendanashworth brendanashworth added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 1, 2015
@Fishrock123
Copy link
Contributor

@cjihrig Looks like the node PR was landed v0.10 by @trevnorris

@Fishrock123
Copy link
Contributor

@cjihrig given this landed in node, is this actually semver-major?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 13, 2015

I would say no. Since it landed in 0.10, I would consider it a bug fix.

@Fishrock123 Fishrock123 removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 13, 2015
cjihrig pushed a commit that referenced this pull request Feb 13, 2015
'.' 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
Copy link
Contributor

cjihrig commented Feb 13, 2015

Thanks. Landed in faa687b

@cjihrig cjihrig closed this Feb 13, 2015
@rvagg rvagg mentioned this pull request Feb 18, 2015
rvagg added a commit that referenced this pull request Feb 20, 2015
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

@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
Copy link
Contributor

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

It doesn't matter if it is a bug fix or not, it's backwards-incompatible.

http://semver.org/

MINOR version when you add functionality in a backwards-compatible manner, and
PATCH version when you make backwards-compatible bug fixes.

Is io.js not following semver now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.