Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Follow the spec: Mappings end at EOL #90

Closed
wants to merge 1 commit into from

4 participants

@lydell

This is to follow the spec.

As discussed in the mailing list, when getting original positions,
mappings end at the end of the line. However, when getting generated
positions, mappings do not end at the end of the line.

Getting original positions:

For a generated position (L,C), find a mapping (L,X) such that X<=C and
X is as great as possible. If no such mapping can be found, then there
is no mapping for (L,C). This is because according to the spec, mappings
end at the end of the line. Previously, we used to look for mappings on
lines <L too.

Getting generated positions:

For a generated position (L,C), find a mapping (A,B) such that A<=L, A
is as great as possible, B<=C and B is as great as possible. If there
are multiple such mappings, I don't know which is or should be chosen.
The spec doesn't say anything about it. The idea is at least that
multiline source statements usually (ideally) only have a mapping at the
beginning of the multiline statement. Then it is convienient to be able
to get the generated position even if you're not at the first line.

Note that this change is backwards incompatible. There were even tests
for the faulty behavior in test/source-map/test-dogfooding.js! I had to
change them, which of course breaks backwards compatibility.

@lydell

I don't get it: when I run npm test locally, the added test fails as expected. But on this page it says that the Travis CI build passed!

@lydell lydell Fix: Generated mappings end at EOL
This is to follow the spec.

As discussed in the [mailing list][1], when getting original positions,
mappings end at the end of the line. However, when getting generated
positions, mappings do not end at the end of the line.

[1]: https://groups.google.com/forum/#!topic/mozilla.dev.js-sourcemap/Uo26hB5v5oY

Getting original positions:

For a generated position (L,C), find a mapping (L,X) such that X<=C and
X is as great as possible. If no such mapping can be found, then there
is no mapping for (L,C). This is because according to the spec, mappings
end at the end of the line. Previously, we used to look for mappings on
lines <L too.

Getting generated positions:

For a generated position (L,C), find a mapping (A,B) such that A<=L, A
is as great as possible, B<=C and B is as great as possible. If there
are multiple such mappings, I don't know which is or should be chosen.
The spec doesn't say anything about it. The idea is at least that
multiline source statements usually (ideally) only have a mapping at the
beginning of the multiline statement. Then it is convienient to be able
to get the generated position even if you're not at the first line.

Note that this change is backwards incompatible. There were even tests
for the faulty behavior in test/source-map/test-dogfooding.js! I had to
change them, which of course breaks backwards compatibility.
699dff1
@lydell

I actually managed to fix this issue, so I amended the fix to the test cases.

@lydell lydell referenced this pull request in sokra/source-map-visualization
Closed

The visualizations in the generated file are misleading: Mappings end at EOL #7

@sokra

SourceNode.fromStringWithSourceMap and SourceNode.prototype.toStringWithSourceMap are wrong too...

@fitzgen
Owner

I don't get it: when I run npm test locally, the added test fails as expected. But on this page it says that the Travis CI build passed!

The test runner had a bug. I fixed it in eabb2e9.

@fitzgen
Owner

SourceNode.fromStringWithSourceMap and SourceNode.prototype.toStringWithSourceMap are wrong too...

Filed #96

@fitzgen
Owner

Merged in d7373b8

@fitzgen fitzgen closed this
@fitzgen
Owner

Thanks @lydell!

@lieut-data

I have a case where mapping.generatedLine is 29 and needle.generatedLine is "029", which fails to pass the strict type check on line 315. Source map is being generated by a version of UglifyJS2.

Relaxing this comparison resolves my issue, but is there any underlying problem to fix instead?

is there any underlying problem to fix instead?

Yes, work with numbers, not strings, of course.

BTW, it is possible to comment on certain lines (instead of writing "on line 315").

Aha! I didn't realize I could comment on a particular line -- thanks for the tip!

I've already monkey patched my version to perform the less strict comparison, but I wasn't sure if there was an even deeper problem that led to the disparate data types. Perhaps it's just an oddity of the various minifiers.

Thanks!

Where does "029" come from?

Chalk this one up to PEBKAC -- upon digging deeper, I realized the source of the incorrect needle line/column values originated in my own code, and that it's perfectly reasonable for the code above to expect a numeric value. Sorry for the distraction!

@lydell lydell deleted the lydell:mappings+eol branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 14, 2014
  1. @lydell

    Fix: Generated mappings end at EOL

    lydell authored
    This is to follow the spec.
    
    As discussed in the [mailing list][1], when getting original positions,
    mappings end at the end of the line. However, when getting generated
    positions, mappings do not end at the end of the line.
    
    [1]: https://groups.google.com/forum/#!topic/mozilla.dev.js-sourcemap/Uo26hB5v5oY
    
    Getting original positions:
    
    For a generated position (L,C), find a mapping (L,X) such that X<=C and
    X is as great as possible. If no such mapping can be found, then there
    is no mapping for (L,C). This is because according to the spec, mappings
    end at the end of the line. Previously, we used to look for mappings on
    lines <L too.
    
    Getting generated positions:
    
    For a generated position (L,C), find a mapping (A,B) such that A<=L, A
    is as great as possible, B<=C and B is as great as possible. If there
    are multiple such mappings, I don't know which is or should be chosen.
    The spec doesn't say anything about it. The idea is at least that
    multiline source statements usually (ideally) only have a mapping at the
    beginning of the multiline statement. Then it is convienient to be able
    to get the generated position even if you're not at the first line.
    
    Note that this change is backwards incompatible. There were even tests
    for the faulty behavior in test/source-map/test-dogfooding.js! I had to
    change them, which of course breaks backwards compatibility.
This page is out of date. Refresh to see the latest.
View
2  lib/source-map/source-map-consumer.js
@@ -312,7 +312,7 @@ define(function (require, exports, module) {
"generatedColumn",
util.compareByGeneratedPositions);
- if (mapping) {
+ if (mapping && mapping.generatedLine === needle.generatedLine) {
var source = util.getArg(mapping, 'source', null);
if (source && this.sourceRoot) {
source = util.join(this.sourceRoot, source);
View
22 test/source-map/test-dog-fooding.js
@@ -42,6 +42,12 @@ define(function (require, exports, module) {
generated: { line: 5, column: 2 }
});
+ smg.addMapping({
+ source: 'gza.coffee',
+ original: { line: 5, column: 10 },
+ generated: { line: 6, column: 12 }
+ });
+
var smc = new SourceMapConsumer(smg.toString());
// Exact
@@ -49,24 +55,30 @@ define(function (require, exports, module) {
util.assertMapping(3, 2, '/wu/tang/gza.coffee', 2, 0, null, smc, assert);
util.assertMapping(4, 2, '/wu/tang/gza.coffee', 3, 0, null, smc, assert);
util.assertMapping(5, 2, '/wu/tang/gza.coffee', 4, 0, null, smc, assert);
+ util.assertMapping(6, 12, '/wu/tang/gza.coffee', 5, 10, null, smc, assert);
// Fuzzy
- // Original to generated
+ // Generated to original
util.assertMapping(2, 0, null, null, null, null, smc, assert, true);
util.assertMapping(2, 9, '/wu/tang/gza.coffee', 1, 0, null, smc, assert, true);
- util.assertMapping(3, 0, '/wu/tang/gza.coffee', 1, 0, null, smc, assert, true);
+ util.assertMapping(3, 0, null, null, null, null, smc, assert, true);
util.assertMapping(3, 9, '/wu/tang/gza.coffee', 2, 0, null, smc, assert, true);
- util.assertMapping(4, 0, '/wu/tang/gza.coffee', 2, 0, null, smc, assert, true);
+ util.assertMapping(4, 0, null, null, null, null, smc, assert, true);
util.assertMapping(4, 9, '/wu/tang/gza.coffee', 3, 0, null, smc, assert, true);
- util.assertMapping(5, 0, '/wu/tang/gza.coffee', 3, 0, null, smc, assert, true);
+ util.assertMapping(5, 0, null, null, null, null, smc, assert, true);
util.assertMapping(5, 9, '/wu/tang/gza.coffee', 4, 0, null, smc, assert, true);
+ util.assertMapping(6, 0, null, null, null, null, smc, assert, true);
+ util.assertMapping(6, 9, null, null, null, null, smc, assert, true);
+ util.assertMapping(6, 13, '/wu/tang/gza.coffee', 5, 10, null, smc, assert, true);
- // Generated to original
+ // Original to generated
util.assertMapping(2, 2, '/wu/tang/gza.coffee', 1, 1, null, smc, assert, null, true);
util.assertMapping(3, 2, '/wu/tang/gza.coffee', 2, 3, null, smc, assert, null, true);
util.assertMapping(4, 2, '/wu/tang/gza.coffee', 3, 6, null, smc, assert, null, true);
util.assertMapping(5, 2, '/wu/tang/gza.coffee', 4, 9, null, smc, assert, null, true);
+ util.assertMapping(5, 2, '/wu/tang/gza.coffee', 5, 9, null, smc, assert, null, true);
+ util.assertMapping(6, 12, '/wu/tang/gza.coffee', 6, 19, null, smc, assert, null, true);
};
});
View
24 test/source-map/test-source-map-consumer.js
@@ -80,6 +80,30 @@ define(function (require, exports, module) {
util.assertMapping(2, 9, '/the/root/two.js', 1, 16, null, map, assert, null, true);
};
+ exports['test mappings and end of lines'] = function (assert, util) {
+ var smg = new SourceMapGenerator({
+ file: 'foo.js'
+ });
+ smg.addMapping({
+ original: { line: 1, column: 1 },
+ generated: { line: 1, column: 1 },
+ source: 'bar.js'
+ });
+ smg.addMapping({
+ original: { line: 2, column: 2 },
+ generated: { line: 2, column: 2 },
+ source: 'bar.js'
+ });
+
+ var map = SourceMapConsumer.fromSourceMap(smg);
+
+ // When finding original positions, mappings end at the end of the line.
+ util.assertMapping(2, 1, null, null, null, null, map, assert, true)
+
+ // When finding generated positions, mappings do not end at the end of the line.
+ util.assertMapping(1, 1, 'bar.js', 2, 1, null, map, assert, null, true);
+ };
+
exports['test creating source map consumers with )]}\' prefix'] = function (assert, util) {
assert.doesNotThrow(function () {
var map = new SourceMapConsumer(")]}'" + JSON.stringify(util.testMap));
Something went wrong with that request. Please try again.