Handle empty root in util.relative

[jgoz]

Re #128, an empty root passed to util.relative should return aPath as-is. This avoids stripping the leading / off of absolute file paths.

Since we don't know whether the empty string is a URL or a file path, we cannot make an assumption either way and should return the path unmodified.

See webpack/core#5 for context around this fix. Specifically, webpack/core#5 (comment) and webpack/core#5 (comment).

[lydell]

Hmm, perhaps empty strings should be treated as ".", just like in util.join. That would be cleaner. If so, please add tests for "." as well. Moreover, it would be good to have a test using the public API that would break without this PR.

[lydell]

Super minor nitpick: Change rootone into root/one. rootone.js is used in the above line just to make sure that libUtil.relative doesn’t think that it should strip /the/root (since both arguments start with that string), because that would be wrong.

[jgoz]

Good point, thanks.

[jgoz]

@lydell

Hmm, perhaps empty strings should be treated as ".", just like in util.join. That would be cleaner

Are you proposing that both util.relative(".", "/foo/bar") and util.relative("", "/foo/bar") should return /foo/bar? This makes logical sense to me, because we have no concept of CWD here, so determining a relative path to an absolute path is not really possible.

Moreover, it would be good to have a test using the public API that would break without this PR

Agreed. The test I added for the empty string would break without this change too.

[jgoz]

@lydell Applied the suggested changes.

The only thing I'm not sure about is this test case. You might argue that it should behave like join and return /the/root/one.js, but this is a weird input combination to pass to relative.

As implemented, using / as the root path will always strip the leading path separator from any supplied path, whether or not the path is absolute. Maybe that consistency is a good thing for these unlikely edge cases.

[fitzgen]

Are you proposing that both util.relative(".", "/foo/bar") and util.relative("", "/foo/bar") should return /foo/bar? This makes logical sense to me, because we have no concept of CWD here, so determining a relative path to an absolute path is not really possible.

Yes, definitely.

Reviewing changes now...

[fitzgen]

The only thing I'm not sure about is this test case. You might argue that it should behave like join and return /the/root/one.js, but this is a weird input combination to pass to relative.

No, that looks good as-is.

[fitzgen]

Thanks for the pull request!

[jgoz]

Thanks for the quick turnaround!

[sokra]

@fitzgen Could you publish it as 0.1.38? It passes all webpack tests now...

[fitzgen]

@sokra done.