Skip to content
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

Don't emit source mappings for removed strings. #159

Merged
merged 1 commit into from Oct 4, 2019
Merged

Don't emit source mappings for removed strings. #159

merged 1 commit into from Oct 4, 2019

Conversation

kzc
Copy link
Contributor

@kzc kzc commented Jul 20, 2019

Fixes #149

Also fixes two incorrect test results and the rollup issue:
rollup/rollup#3001

@kzc
Copy link
Contributor Author

kzc commented Jul 20, 2019

@Rich-Harris Please take a look. If the approach in this PR is incorrect you can use the following to verify your results for a different fix:

Here are the dumps of the original incorrect test source maps using source-map@0.6.1:

$ cat dump-original-test-sourcemaps.js 
[
    '{"version":3,"file":"output.md","sources":["input.md"],"sourcesContent":["abcdefghijkl"],"names":[],"mappings":"AAAA,CAAC,CAAC,CAAC,AAAM,CAAC,CAAC"}',
    '{"version":3,"file":"output.md","sources":["input.md"],"sourcesContent":["abcdefghijkl"],"names":[],"mappings":"AAAA,GAAG,GAAG,AAAG,CAAC"}',
].forEach(function(json) {
    console.log(json);
    var sourcemap = JSON.parse(json);
    var consumer = new require("source-map").SourceMapConsumer(sourcemap);
    consumer.eachMapping(m => console.log(JSON.stringify(m)));
    console.log("-----------------------");
});
$ cat dump-original-test-sourcemaps.js | node
{"version":3,"file":"output.md","sources":["input.md"],"sourcesContent":["abcdefghijkl"],"names":[],"mappings":"AAAA,CAAC,CAAC,CAAC,AAAM,CAAC,CAAC"}
{"source":"input.md","generatedLine":1,"generatedColumn":0,"originalLine":1,"originalColumn":0,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":1,"originalLine":1,"originalColumn":1,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":2,"originalLine":1,"originalColumn":2,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":3,"originalLine":1,"originalColumn":3,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":3,"originalLine":1,"originalColumn":9,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":4,"originalLine":1,"originalColumn":10,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":5,"originalLine":1,"originalColumn":11,"name":null}
-----------------------
{"version":3,"file":"output.md","sources":["input.md"],"sourcesContent":["abcdefghijkl"],"names":[],"mappings":"AAAA,GAAG,GAAG,AAAG,CAAC"}
{"source":"input.md","generatedLine":1,"generatedColumn":0,"originalLine":1,"originalColumn":0,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":3,"originalLine":1,"originalColumn":3,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":6,"originalLine":1,"originalColumn":6,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":6,"originalLine":1,"originalColumn":9,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":7,"originalLine":1,"originalColumn":10,"name":null}
-----------------------

Notice the collisions of the generated line/column combinations for the respective tests above:

{"source":"input.md","generatedLine":1,"generatedColumn":3,"originalLine":1,"originalColumn":3,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":3,"originalLine":1,"originalColumn":9,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":6,"originalLine":1,"originalColumn":6,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":6,"originalLine":1,"originalColumn":9,"name":null}

Here are the dumps of the altered source map tests in this PR without the collisions and without the mappings for the dropped characters:

$ cat dump-new-test-sourcemaps.js 
[
    '{"version":3,"file":"output.md","sources":["input.md"],"sourcesContent":["abcdefghijkl"],"names":[],"mappings":"AAAA,CAAC,CAAC,CAAO,CAAC,CAAC"}',
    '{"version":3,"file":"output.md","sources":["input.md"],"sourcesContent":["abcdefghijkl"],"names":[],"mappings":"AAAA,GAAG,GAAM,CAAC"}',
].forEach(function(json) {
    console.log(json);
    var sourcemap = JSON.parse(json);
    var consumer = new require("source-map").SourceMapConsumer(sourcemap);
    consumer.eachMapping(m => console.log(JSON.stringify(m)));
    console.log("-----------------------");
});
$ cat dump-new-test-sourcemaps.js | node
{"version":3,"file":"output.md","sources":["input.md"],"sourcesContent":["abcdefghijkl"],"names":[],"mappings":"AAAA,CAAC,CAAC,CAAO,CAAC,CAAC"}
{"source":"input.md","generatedLine":1,"generatedColumn":0,"originalLine":1,"originalColumn":0,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":1,"originalLine":1,"originalColumn":1,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":2,"originalLine":1,"originalColumn":2,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":3,"originalLine":1,"originalColumn":9,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":4,"originalLine":1,"originalColumn":10,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":5,"originalLine":1,"originalColumn":11,"name":null}
-----------------------
{"version":3,"file":"output.md","sources":["input.md"],"sourcesContent":["abcdefghijkl"],"names":[],"mappings":"AAAA,GAAG,GAAM,CAAC"}
{"source":"input.md","generatedLine":1,"generatedColumn":0,"originalLine":1,"originalColumn":0,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":3,"originalLine":1,"originalColumn":3,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":6,"originalLine":1,"originalColumn":9,"name":null}
{"source":"input.md","generatedLine":1,"generatedColumn":7,"originalLine":1,"originalColumn":10,"name":null}
-----------------------

Fixes #149

Also fixes two incorrect test results and the rollup issue:
rollup/rollup#3001
Copy link
Collaborator

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response — the fix looks great to me! Although I don't fully understand what's going on in the mappings code in general, so it would be great to get another pair of eyes on this — @guybedford maybe?

@obenjiro
Copy link

obenjiro commented Aug 5, 2019

Any news on this one?

@mourner mourner merged commit 0c4d32f into Rich-Harris:master Oct 4, 2019
@kzc kzc deleted the dont-emit-mappings-for-removed branch October 4, 2019 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]
3 participants