Add a third parameter to `applySourceMap` #93

Merged
merged 4 commits into from Feb 26, 2014

3 participants

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

@lydell lydell Add `util.normalize()` and let `util.join()` use it
Use case: `util.join('a', '../b')` should be `'b'`, not `'a/../b'`.
d281271
@fitzgen
Mozilla member

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 sokra and 2 others commented on an outdated diff Feb 14, 2014
test/source-map/test-util.js
+
+ assert.equal(lib_util.normalize('.///.././../a/b//./..'), '../../a')
+
+ assert.equal(lib_util.normalize('http://www.example.com'), 'http://www.example.com');
+ assert.equal(lib_util.normalize('http://www.example.com/'), 'http://www.example.com/');
+ assert.equal(lib_util.normalize('http://www.example.com/./..//a/b/c/.././d//'), 'http://www.example.com/a/b/d/');
+ };
+
+ exports['test join()'] = function (assert, util) {
+ assert.equal(lib_util.join('a', 'b'), 'a/b');
+ assert.equal(lib_util.join('a/', 'b'), 'a/b');
+ assert.equal(lib_util.join('a//', 'b'), 'a/b');
+ assert.equal(lib_util.join('a', 'b/'), 'a/b/');
+ assert.equal(lib_util.join('a', 'b//'), 'a/b/');
+ assert.equal(lib_util.join('a/', '/b'), 'a/b');
+ assert.equal(lib_util.join('a//', '//b'), 'a/b');
@sokra
sokra added a note Feb 14, 2014

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

@lydell
lydell added a note Feb 14, 2014

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

@fitzgen
Mozilla member
fitzgen added a note Feb 14, 2014

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
sokra added a note Feb 15, 2014

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");
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fitzgen fitzgen commented on the diff Feb 14, 2014
test/source-map/test-source-map-generator.js
+ // public/
+ // - bundle.min.js
+ // - bundle.min.js.map
+
+ var bundleMap = new SourceMapGenerator({
+ file: 'bundle.js'
+ });
+ bundleMap.addMapping({
+ generated: { line: 3, column: 3 },
+ original: { line: 2, column: 2 },
+ source: '../coffee/foo.coffee'
+ });
+ bundleMap.addMapping({
+ generated: { line: 13, column: 13 },
+ original: { line: 12, column: 12 },
+ source: '/bar.coffee'
@fitzgen
Mozilla member
fitzgen added a note Feb 14, 2014

Nitpick: /bar.coffee isn't present in the directory structure comment, you should probably add it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fitzgen fitzgen commented on the diff Feb 14, 2014
test/source-map/test-source-map-generator.js
+ file: 'bundle.js'
+ });
+ bundleMap.addMapping({
+ generated: { line: 3, column: 3 },
+ original: { line: 2, column: 2 },
+ source: '../coffee/foo.coffee'
+ });
+ bundleMap.addMapping({
+ generated: { line: 13, column: 13 },
+ original: { line: 12, column: 12 },
+ source: '/bar.coffee'
+ });
+ bundleMap.addMapping({
+ generated: { line: 23, column: 23 },
+ original: { line: 22, column: 22 },
+ source: 'http://www.example.com/baz.coffee'
@fitzgen
Mozilla member
fitzgen added a note Feb 14, 2014

Nor is baz.coffee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fitzgen fitzgen and 1 other commented on an outdated diff Feb 14, 2014
test/source-map/test-source-map-generator.js
+ source: 'temp/bundle.js'
+ });
+ minifiedMap.addMapping({
+ generated: { line: 21, column: 21 },
+ original: { line: 23, column: 23 },
+ source: 'temp/bundle.js'
+ });
+
+ var expectedMap = new SourceMapGenerator({
+ file: 'bundle.min.js',
+ sourceRoot: '..'
+ });
+ expectedMap.addMapping({
+ generated: { line: 1, column: 1 },
+ original: { line: 2, column: 2 },
+ source: 'coffee/foo.coffee'
@fitzgen
Mozilla member
fitzgen added a note Feb 14, 2014

I think that having the source for the expected map's mappings be an absolute URL makes the expected map a little easier to read. You don't have to do the path joining and normalizing in your head and gives us a reference for the other map. Can you change the first two mappings to be absolute URLs like the third one is now? Thanks.

@lydell
lydell added a note Feb 15, 2014

Sorry, I don't quite follow here. We need to test relative paths, so all of them can't be absolute. And only the first one is relative, not the first two?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fitzgen fitzgen commented on an outdated diff Feb 14, 2014
test/source-map/test-source-map-generator.js
+ source: 'coffee/foo.coffee'
+ });
+ expectedMap.addMapping({
+ generated: { line: 11, column: 11 },
+ original: { line: 12, column: 12 },
+ source: '/bar.coffee'
+ });
+ expectedMap.addMapping({
+ generated: { line: 21, column: 21 },
+ original: { line: 22, column: 22 },
+ source: 'http://www.example.com/baz.coffee'
+ });
+
+ minifiedMap.applySourceMap(
+ new SourceMapConsumer(bundleMap.toJSON()),
+ // Note that relying on `bundleMap.file` (which is simply 'bundle.js') wouldn't work here.
@fitzgen
Mozilla member
fitzgen added a note Feb 14, 2014

Nitpick: this comment is a little long, mind wrapping it at 80 chars?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fitzgen fitzgen commented on an outdated diff Feb 14, 2014
test/source-map/test-util.js
@@ -0,0 +1,72 @@
+/* -*- Mode: js; js-indent-level: 2; -*- */
+/*
+ * Copyright 2014 Mozilla Foundation and contributors
+ * Licensed under the New BSD license. See LICENSE or:
+ * http://opensource.org/licenses/BSD-3-Clause
+ */
+if (typeof define !== 'function') {
+ var define = require('amdefine')(module, require);
+}
+define(function (require, exports, module) {
+
+ var lib_util = require('../../lib/source-map/util');
@fitzgen
Mozilla member
fitzgen added a note Feb 14, 2014

Nitpick: s/lib_util/libUtil/ please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fitzgen fitzgen commented on an outdated diff Feb 14, 2014
test/source-map/test-util.js
+ assert.equal(lib_util.normalize('a/b//c////d'), 'a/b/c/d');
+
+ assert.equal(lib_util.normalize('.///.././../a/b//./..'), '../../a')
+
+ assert.equal(lib_util.normalize('http://www.example.com'), 'http://www.example.com');
+ assert.equal(lib_util.normalize('http://www.example.com/'), 'http://www.example.com/');
+ assert.equal(lib_util.normalize('http://www.example.com/./..//a/b/c/.././d//'), 'http://www.example.com/a/b/d/');
+ };
+
+ exports['test join()'] = function (assert, util) {
+ assert.equal(lib_util.join('a', 'b'), 'a/b');
+ assert.equal(lib_util.join('a/', 'b'), 'a/b');
+ assert.equal(lib_util.join('a//', 'b'), 'a/b');
+ assert.equal(lib_util.join('a', 'b/'), 'a/b/');
+ assert.equal(lib_util.join('a', 'b//'), 'a/b/');
+ assert.equal(lib_util.join('a/', '/b'), 'a/b');
@fitzgen
Mozilla member
fitzgen added a note Feb 14, 2014

Joining these two strings should actually result in "/b". I know this is an existing bug, but it should be pretty easy to include in your patch. If you don't want to do the extra work, either file a bug for it or let me know and I will file a bug for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fitzgen
Mozilla member

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

@lydell lydell Add a third parameter to `applySourceMap`
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.
690286b
@lydell

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

@lydell lydell Enhance `util.join(aRoot, aPath)`
- If aPath is a URL or a data URI, aPath is returned, unless aPath is a
  scheme-relative URL: Then the scheme of aRoot, if any, is prepended
  first.
- Otherwise aPath is a path. If aRoot is a URL, then its path portion
  is updated with the result and aRoot is returned. Otherwise the result
  is returned.
  - If aPath is absolute, the result is aPath.
  - Otherwise the two paths are joined with a slash.
- Joining for example 'http://' and 'www.example.com' is also supported.
52500e0
@lydell

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

@lydell lydell commented on an outdated diff Feb 16, 2014
lib/source-map/source-map-generator.js
@@ -179,10 +185,12 @@ define(function (require, exports, module) {
});
if (original.source !== null) {
// Copy mapping
+ mapping.source = original.source;
+ if (aSourceMapPath && mapping.source.charAt(0) !== '/') {
@lydell
lydell added a note Feb 16, 2014

This should probably be simplified to if (aSourceMapPath).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@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
Mozilla member

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 fitzgen merged commit 4dce61e into mozilla:master Feb 26, 2014
@fitzgen
Mozilla member

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.

@lydell lydell referenced this pull request Mar 1, 2014
Open

Wrong sourceRoot handling? #62

@lydell lydell deleted the lydell:apply-source-map branch Jun 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment