Use WHATWG's URL to implement all of source-map's URL operations. #367

loganfsmyth · 2018-10-15T18:14:15Z

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 and util.join which seems less than ideal.

As mentioned in a comment in the code, this also uses an npm dependency instead of the global URL because I found at least one case where FF's URL seems to not follow the spec at the moment. This way we can be confident for now that things actually work across platforms. The whatwg-url library is MIT. Given that source-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.

tromey · 2018-10-16T19:35:23Z

test/test-util.js

@@ -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");


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.

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.

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.

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.

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.

tromey

There were a few instances of that change. I think they all have to be fixed.

fitzgen · 2018-10-18T13:56:18Z

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 :)

coveralls · 2018-10-18T21:09:51Z

Pull Request Test Coverage Report for Build 524

87 of 89 (97.75%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.7%) to 87.457%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/util.js	87	89	97.75%

Totals
Change from base Build 518:	-0.7%
Covered Lines:	817
Relevant Lines:	913

💛 - Coveralls

loganfsmyth · 2018-10-22T20:53:21Z

@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?

tromey · 2018-10-23T17:06:56Z

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.

tromey · 2018-10-23T17:09:17Z

test/test-source-map-consumer.js

@@ -1049,26 +1049,6 @@ exports["test sourceRoot + originalPositionFor"] = async function(assert) {
  map.destroy();
 };

-exports["test github issue #56"] = async function(assert) {


It seems to me that this test should still work and should not be removed.

I added a new sourceRoot example here that should show this in action. Works in Chrome.

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.

tromey · 2018-10-23T17:17:35Z

test/test-util.js

-  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/////");


Not sure it matters but this doesn't seem like a "normal" form. What's the deal here?

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"

tromey · 2018-10-23T17:19:12Z

test/test-util.js


-  assert.equal(libUtil.normalize("http://www.example.com"), "http://www.example.com");
+  assert.equal(libUtil.normalize(".///.././../a/b//./.."), "a/b/");


The original result here seems more correct than this one to me. What's the rationale for this change?

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.

tromey · 2018-10-23T17:21:07Z

lib/util.js

+  // 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


Typo, "achieve"

loganfsmyth · 2018-10-23T22:54:24Z

@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 source-map as a library for consumers to use. I probably came into this focused more on the library side, and thus more willing to drop support for things that seem like underdefined edge cases. I've had some frustrations with source-map at times and was hoping to really make source-map a solid base. As it is, there are several re-implementations of sourcemap parsing, and they all do things slightly differently around root + source handling. It seems like the community as a whole would be well-served by a well-defined, common library for consuming sourcemaps.

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 that probably need a big overhaul that complicates things. Primarily, if we're expecting sourceRoot to be opaque, then realistically util.relative, util.normalize, and util.join should be avoided at all costs in the generator and source-node files. For instance, if sourceRoot isn't a URL, then given something like

source-map/lib/source-map-generator.js

Line 57 in 7356bcb

newMapping.source = util.relative(sourceRoot, newMapping.source);

, how should that that be computed? It technically works right now, but that's implemented as a mess of custom string processing that is implementation-defined custom logic. That code would need to be updates to use the original 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

mapping.source = util.relative(sourceRoot, mapping.source);

There are actually existing bugs here, because there are just some maps that can't be generated safely. For example

const { SourceMapConsumer, SourceMapGenerator } = require(".");

(async function() {
  const module = await new SourceMapConsumer({
    version: 3,
    file: "built-module.js",
    sourceRoot: "webpack:///folder/",
    sources: ["module.js"],
    mappings: "AAAA",
  });

  const gen = new SourceMapGenerator({
    sourceRoot: "other/",
  });
  gen.addMapping({
    source: "built-module.js",
    generated: { line: 1, column: 0 },
    original: { line: 1, column: 0 },
  });
  gen.addMapping({
    source: "index.js",
    generated: { line: 2, column: 0 },
    original: { line: 1, column: 0 },
  });

  gen.applySourceMap(module);

  const result = gen.toJSON();
  console.log(result);
  //  { version: 3,
  //    sources: [ 'webpack:///folder/module.js', 'index.js' ],
  //    names: [],
  //    mappings: 'AAAA;ACAA',
  //    sourceRoot: 'other/' }
})();

produces a map where the joined path is other/webpack:///folder/module.js which is incorrect given that the concatenated root+source was an absolute URL. The call to applySourceMap would need to delete the sourceRoot value and change the sources entry to other/index.js.

fitzgen

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

jasonLaster · 2018-11-08T17:42:11Z

LRU caching URL parser

anytime you can have a palindrome you should...

loganfsmyth · 2018-11-15T22:05:01Z

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.

Use WHATWG's URL to implement all of source-map's URL operations.

e38385b

loganfsmyth requested review from fitzgen and tromey October 15, 2018 18:14

loganfsmyth mentioned this pull request Oct 15, 2018

Use standard-conforming URL #360

Closed

tromey reviewed Oct 16, 2018

View reviewed changes

tromey suggested changes Oct 16, 2018

View reviewed changes

fitzgen removed their request for review October 18, 2018 13:55

Preserve string-concat sources behavior for absolute-path sources.

778cb51

mozilla deleted a comment from coveralls Oct 18, 2018

tromey reviewed Oct 23, 2018

View reviewed changes

fitzgen reviewed Nov 5, 2018

View reviewed changes

loganfsmyth closed this Nov 13, 2018

loganfsmyth changed the base branch from normalize-consumer-urls to master November 13, 2018 23:03

loganfsmyth mentioned this pull request Nov 15, 2018

Use WHATWG's URL to implement all of source-map's URL operations. #371

Merged

jasonLaster mentioned this pull request Dec 26, 2018

Bug 7571 - set directory root for debugger.html firefox-devtools/debugger#7594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use WHATWG's URL to implement all of source-map's URL operations. #367

Use WHATWG's URL to implement all of source-map's URL operations. #367

loganfsmyth commented Oct 15, 2018

tromey Oct 16, 2018

loganfsmyth Oct 16, 2018 •

edited

loganfsmyth Oct 16, 2018

tromey Oct 17, 2018

loganfsmyth Oct 18, 2018

tromey left a comment

fitzgen commented Oct 18, 2018

coveralls commented Oct 18, 2018

loganfsmyth commented Oct 22, 2018

tromey commented Oct 23, 2018

tromey Oct 23, 2018

tromey Oct 23, 2018

loganfsmyth Oct 23, 2018

tromey Oct 23, 2018

loganfsmyth Oct 23, 2018

tromey Oct 23, 2018

loganfsmyth Oct 23, 2018

tromey Oct 23, 2018

loganfsmyth commented Oct 23, 2018

fitzgen left a comment

jasonLaster commented Nov 8, 2018

loganfsmyth commented Nov 15, 2018 •

edited


		assert.equal(libUtil.normalize("http://www.example.com"), "http://www.example.com");
		assert.equal(libUtil.normalize(".///.././../a/b//./.."), "a/b/");

Use WHATWG's URL to implement all of source-map's URL operations. #367

Use WHATWG's URL to implement all of source-map's URL operations. #367

Conversation

loganfsmyth commented Oct 15, 2018

Choose a reason for hiding this comment

loganfsmyth Oct 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromey left a comment

Choose a reason for hiding this comment

fitzgen commented Oct 18, 2018

coveralls commented Oct 18, 2018

Pull Request Test Coverage Report for Build 524

💛 - Coveralls

loganfsmyth commented Oct 22, 2018

tromey commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loganfsmyth commented Oct 23, 2018

fitzgen left a comment

Choose a reason for hiding this comment

jasonLaster commented Nov 8, 2018

loganfsmyth commented Nov 15, 2018 • edited

loganfsmyth Oct 16, 2018 •

edited

loganfsmyth commented Nov 15, 2018 •

edited