-
Notifications
You must be signed in to change notification settings - Fork 358
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
Use WHATWG's URL to implement all of source-map's URL operations. #367
Conversation
test/test-util.js
Outdated
@@ -236,7 +193,7 @@ exports["test computeSourceURL"] = function(assert) { | |||
assert.equal(libUtil.computeSourceURL("src/", "test.js", "http://example.com"), | |||
"http://example.com/src/test.js"); | |||
assert.equal(libUtil.computeSourceURL("src", "/test.js", "http://example.com"), | |||
"http://example.com/src/test.js"); | |||
"http://example.com/test.js"); |
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.
I think this change is wrong according to the spec. It says:
Line 4: An optional source root, useful for relocating source files on a server or removing repeated values in the “sources” entry. This value is prepended to the individual entries in the “source” field.
Then later:
If the sources are not absolute URLs after prepending of the “sourceRoot”, the sources are resolved relative to the SourceMap (like resolving script src in a html document).
That is, the source root and the source are joined with string concatenation; then URL resolution is done using the source map's URL. Here, IIUC, the source root is src
and the source is /test.js
, so by my reading the test was correct before this change.
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.
This is where I'm on the fence too. It depends on if we want to interpret prepend
. Doing a straight string concatenation seems terrible, but it's hard to see the existing logic of adding a /
if there isn't one as anything but super confusing spec-wise too, since the spec also never mentions prepending with a /
when needed. So if we accept that it's supposed to be a url-like concatenation, then an absolute path does override the value in the root.
I'm fine reverting this behavior, but it does seem like this would be the ideal behavior if possible.
What I kind of want to do is land this in Nightly and see if we get complaints about broken sourcemaps, but I'm not sure if that's a good approach or what. I have a hard time thinking of a case where someone's going to have a sources
entry starting with /
that isn't an absolute path, but you're not wrong that it could technically exist.
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.
I guess my feeling on it is that the spec is extremely under-specified, and as maintainers of this library we should use it to shepard the spec into a clearly-defined state.
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.
The spec leaves a lot to be desired, and this was a questionable choice, but I don't think this one is ambiguous. I think if you dig through bugzilla (look at the closed bugs that block the source-maps bug) you'll find some real-world cases that motivated changes here. I suggest doing this, and also looking through the source map examples page, before making breaking changes. Also cross-testing against Chrome is useful, as Chrome's source map support is generally better than Firefox's.
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.
Fair. I see discussions in https://bugzilla.mozilla.org/show_bug.cgi?id=977463 and Lydell pretty much stating exactly what I'd have wanted behavior-wise in #62 (comment). Alas :( At least I know I'm not alone in finding the behavior frustrating.
I'm going to add a change to this PR to see if we can find a good intermediate.
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.
There were a few instances of that change. I think they all have to be fixed.
Removing myself from review request since tromey hopped on it, and only one of us ever needs to r+ a PR since I trust his judgement :) |
Pull Request Test Coverage Report for Build 524
💛 - Coveralls |
@tromey I realized I never left a comment, but this should be ready for another pass. I think the new commit I added should address the core of the joining issue while still allowing broader support for URL-like things. Thoughts? |
To my surprise I found that Chrome also has this weird reading of the "append" text, see https://codereview.chromium.org/13898010/diff/5001/Source/devtools/front_end/SourceMap.js. Confirmed by writing a test case. |
@@ -1049,26 +1049,6 @@ exports["test sourceRoot + originalPositionFor"] = async function(assert) { | |||
map.destroy(); | |||
}; | |||
|
|||
exports["test github issue #56"] = async function(assert) { |
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.
It seems to me that this test should still work and should not be removed.
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.
I added a new sourceRoot
example here that should show this in action. Works in Chrome.
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.
It's a breaking change, you're not wrong, but it also seems like something that isn't necessary to support. I have a super hard time imagining a case where anyone is actually going to do this. Webpack does
sourceRoot: "webpack:///",
sources: ["/foo/bar.js"],
but that will work fine with the existing code. It seems very unlikely that someone would split up a scheme and a host into two separate pieces in a real map, and if they do that seems like something we should be discouraging.
assert.equal(libUtil.normalize("/a/b//c////d/////"), "/a/b/c/d/"); | ||
assert.equal(libUtil.normalize("///a/b//c////d/////"), "///a/b/c/d/"); | ||
assert.equal(libUtil.normalize("a/b//c////d"), "a/b/c/d"); | ||
assert.equal(libUtil.normalize("/a/b//c////d/////"), "/a/b//c////d/////"); |
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.
Not sure it matters but this doesn't seem like a "normal" form. What's the deal here?
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.
Multiple slashes in URLs have meaning, they are just empty URL pieces. For example, ../../file.js
relative to http://example.com/a/b/c/d///
is http://example.com/a/b/c/d/file.js
, not http://example.com/a/b/file.js
.
new URL("../../file.js", "http://example.com/a/b/c/d///").href;
// "http://example.com/a/b/c/d/file.js"
|
||
assert.equal(libUtil.normalize("http://www.example.com"), "http://www.example.com"); | ||
assert.equal(libUtil.normalize(".///.././../a/b//./.."), "a/b/"); |
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.
The original result here seems more correct than this one to me. What's the rationale for this change?
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.
Similar to above, the slashes have meaning.
new URL(".///.././../a/b//./..", "http://example.com/1/2/3/4/").href
// "http://example.com/1/2/3/4/a/b/"
so the path is a/b/
relative to the base.
// must behave as "some-dir/some-path.js". | ||
// | ||
// With this library's the transition to a more URL-focused implementation, that behavior is | ||
// preserved here. To acheive that, we trim the "/" from absolute-path when a sourceRoot value |
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.
Typo, "achieve"
@tromey Would you be up for picking a time to talk over this stuff? If my proposed changes are too ambitious, at the end of the day I understand, either way I'd like to settle on something so we can move forward. I think things are made more difficult because there are kind of two sides to source-maps in general. On one hand, we have Firefox which understandably wants to be permissive and consistent with Chrome. On the other side, we have My biggest issue really comes down to the fact that treating the "sourceRoot" and "sources" values as opaque strings that only have meaning after concatenation means that there are other pieces of source-map/lib/source-map-generator.js Line 57 in 7356bcb
sources value from the consumer's input, rather than the joined root + source value exposed as .sources on the consumer. Another example is source-map/lib/source-map-generator.js Line 215 in 7356bcb
produces a map where the joined path is |
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.
LGTM -- can you run the benchmarks with and without this PR to double check that removing the LRU cache doesn't regress perf? We might want to keep that and wrap it around the URL parsing.
In which case we would have an LRU caching URL parser ;-p
anytime you can have a palindrome you should... |
This got auto-closed because I deleted its base branch. Github won't let me reopen even though I fixed that, so I've opened #371. |
This is my proposed approach for #275. While #360 is a step in the right direction, it still leaves all of the custom logic in
util.relative
andutil.join
which seems less than ideal.As mentioned in a comment in the code, this also uses an
npm
dependency instead of the globalURL
because I found at least one case where FF'sURL
seems to not follow the spec at the moment. This way we can be confident for now that things actually work across platforms. Thewhatwg-url
library is MIT. Given thatsource-map
is currently BSD, this might also be another motivation for #362 since I don't know how distributing a bundle that is a mix of BSD and MIT code works?This PR definitely qualifies as breaking, so a good review would be very appreciated.