Add a third parameter to applySourceMap

[lydell]

The paths in the sources property of source maps may be relative. If
so, they are relative to the source map itself. When you apply a source
map A to another source map B, the source in map B that is associated
with map A will be replaced with sources from map A. If those sources
are relative, they need to be rewritten to be relative to map B instead
of map A. In order to do that, the applySourceMap method needs more
information: The missing piece between map B and map A. It is now
possible to give this information, using a third parameter: The path to
map A, relative to map B. (It can also be absolute, of course.)

This should be backwards compatible. If the third parameter is omitted,
the method works exactly like before: It assumes that both maps are in
the same directory, which makes all sources relative to both maps, which
in turn requires no rewriting.

[fitzgen]

Thanks for the detailed description. I'll take a look at this tomorrow.

[lydell]

Crap. I just found a mistake, that somehow slipped through the tests. I'll fix it ASAP, hopefully this weekend.

[sokra]

I would favor to not have an object {from: ..., to: ...}, but only a relative directory where B is relative to A.

from	to	relative
/dir	/dir	. or omit
/dir/a	/dir/b	../b
/dir	/dir/b	b
/dir/a/sub	/dir	../..
/dir	/dir/b/sub	b/sub

Additionally an absolute url should be supported as 3rd argument.

[lydell]

I actually considered that first—but the other way around: A directory either absolute or relative from B to A. Isn't that what you meant? I don't understand how to construct the source paths otherwise.

The reason I didn't go that way was because using {to: ..., from: ...} we can get simpler paths in some cases:

Consider this directory structure:

• dir/
  • foo.js
  • foo.js.map
  • a/
    • b/
      • foo.coffee
      • foo.min.js
      • foo.min.js.map

Using ../.. as the third parameter would produce ../../a/b/foo.coffee as a source in foo.min.js.map, while using {from: 'dir', to: 'dir/a/b'} would simply result in foo.coffee.

Or this directory structure:

• temp/
  • foo.js
  • foo.js.map
• public/
  • foo.coffee
  • foo.min.js
  • foo.min.js.map

../temp -> ../public/foo.coffee vs. {from: 'temp', to: 'public'} -> foo.coffee

But those kind of cases are the only cases I can think of that would produce less than optimal source paths: When you first "climb up" a bit, and then a source happens to "climb down" a bit again. And that doesn't feel like a common use case. A single path to the dirname of map A relative to map B would solve autoprefixer's use cases/issues as well as the test case of this PR exactly as well as {to: ..., from: ...}.

So now it feels like a silly optimization. @sokra's idea makes a much simpler and easier to understand API, and it would also solve the mistake I mentioned in a much simpler way than I had in mind.

@fitzgen, I'm gonna make this change, rebase and force push, so don't waste any time reviewing this right now (although a lot will of course be the same).

[lydell]

Done. Ready for feedback again :)

[sokra]

Isn't //b a protocol-relative url and should be threaded as absolute url? @fitzgen

[lydell]

That's possible. When I wrote these tests I played with the cd command and typing URLs in Firefox.

[fitzgen]

Good catch @sokra. Yes, this should result in "/b". Can you add this assertion as well:

assert.equal(libUtil.join("http://example.com/foo", "//bar"), "http://example.com/bar"); 

Thanks.

[sokra]

I actually should be this way:

assert.equal(libUtil.join('a/', '/b'), '/b'); assert.equal(libUtil.join("http://example.com/foo", "/bar"), "http://example.com/bar"); assert.equal(libUtil.join("http://example.com/foo", "//bar"), "http://bar"); assert.equal(libUtil.join("https://example.com/foo", "//bar.com/foo"), "https://bar.com/foo");

[fitzgen]

Thanks for the PR :) Will merge it when you've updated the patch based on comments above.

[lydell]

I've amended fixes for the nitpicks/code style stuff. Starting to work on util.join() fixes now.

[lydell]

There we go, I've addressed everything except #93 (comment) which needs clarification and discussion. Ready for another round of feedback :)

[lydell]

This should probably be simplified to if (aSourceMapPath).

[lydell]

I might have addressed #93 (comment) now.

[lydell]

lib/source-map/util.js is possibly a bit long now; Should we move urlParse, urlGenerate, normalize, join and relative into its own file (called perhaps url.js or path.js)?

[fitzgen]

Just a note: I probably won't be able to get to this pull request this week because I'm at a team offsite all week.

[lydell]

Hmm ... it would really be a lot easier if we could use the Node.js 'url' module. It’s on npm: https://www.npmjs.org/package/url. Is this out of question?

[lydell]

Really, it would be cool if we could use require("url") in Node.js, use resolve-url in the browser and Components.utils.import(foo) inside Firefox (I hope there is such a thing). Or am I dreaming? All of those do what we want: Resolve urls. Either case util.join should probably be called util.resolve. Currently, util.join is more of a mix between path.join and url.resolve. For example, util.join("../temp/app.js.map", "app.coffee") should be "../temp/app.coffee", not "../temp/app.js.map/app.coffee". This would simplify the description of the third parameter of .applySourceMap() as well: Instead of being the dirname of the path to the source map, it’d just be the path to the source map.

[lydell]

Thinking a bit more about it, I think we should do one of the following:

• Rename util.join to util.resolve, and adjust it slightly so that it resolves like browsers (like Node.js url.resolve does). Pros: Mostly done. Cons: Breaks the unix philosophy, forces us to work on stuff we don’t care about the most (resolving urls instead of source maps).
• Remove util.join and use some other library. Pros: Moves url resolving out of the way, more likely to get a robust, well-defined resolver. Cons: How to deal with different environments and building and stuff?

And then we should adjust the third argument of the .applySourceMap method (that this PR is really about) to be a path to a source map instead of the dirname of a source map. Much more sensible, in my opinion.

[fitzgen]

It would make sense to have a pluggable join/resolve thing going on, but it would need to be integrated with the build system. Feel free to file an issue to track progress.