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.

Sign up for GitHub

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

Jump to bottom

Switch to GenMapping for sourcemap generation #1190

Merged
merged 7 commits into from May 10, 2022
Merged

Switch to GenMapping for sourcemap generation #1190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d23354e
Switch to GenMapping for sourcemap generation
jridgewell Apr 30, 2022
04953b4
Apply suggestions from code review
jridgewell May 1, 2022
8f938df
Fix dependency
jridgewell May 1, 2022
f6dbb8f
Switch to source-map compatible @jridgewell/source-map
jridgewell May 1, 2022
b2e5a12
Update TS types
jridgewell May 1, 2022
6cab7f2
Support source-map's async SourceMapConsumer
jridgewell May 2, 2022
2cc6270
Fix sourcesContent check with source-map's SourceMapGenerator
jridgewell May 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 20 additions & 13 deletions lib/minify.js
View file
Open in desktop
Expand Up @@ -271,18 +271,15 @@ async function minify(files, options, _fs_module) {
}
if (!HOP(options.format, "code") || options.format.code) {
if (options.sourceMap) {
if (options.sourceMap.includeSources && files instanceof AST_Toplevel) {
throw new Error("original source content unavailable");
}
options.format.source_map = await SourceMap({
file: options.sourceMap.filename,
orig: options.sourceMap.content,
root: options.sourceMap.root
root: options.sourceMap.root,
files: options.sourceMap.includeSources ? files : null,
});
if (options.sourceMap.includeSources) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This section was moved into the SourceMap. Before, calling setSourceContent for the original source contents and the input contents would mean there are 2 contents stored because they have different file names. SourceMapGenerator would traverse all mappings and determine if a particular file isn't referenced in the mappings, and would exclude that file's contents from the output.

GenMapping doesn't do that traversal. If you tell it that file.js has contents, then file.js will be in the output map even if it's not actually referenced by any mappings. By moving this into SourceMap, we can do a property lookup for source contents as they're needed.

if (files instanceof AST_Toplevel) {
throw new Error("original source content unavailable");
} else for (var name in files) if (HOP(files, name)) {
options.format.source_map.get().setSourceContent(name, files[name]);
}
}
}
delete options.format.ast;
delete options.format.code;
Expand All @@ -291,11 +288,21 @@ async function minify(files, options, _fs_module) {
toplevel.print(stream);
result.code = stream.get();
if (options.sourceMap) {
if(options.sourceMap.asObject) {
result.map = options.format.source_map.get().toJSON();
} else {
result.map = options.format.source_map.toString();
}
Object.defineProperty(result, "map", {
configurable: true,
enumerable: true,
get() {
const map = options.format.source_map.getEncoded();
return (result.map = options.sourceMap.asObject ? map : JSON.stringify(map));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Encoded maps require serialization from decoded form into an encoded VLQ string. I've placed this behind a getter so that it's not performed unless required by the dev.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated suggestion:

I looked at the implementation underneath getEncoded here and I wonder if a JSON string rendition of it could be optimized.

VLQ strings are guaranteed to not contain \ or " so for large maps it may be more memory efficient to generate the JSON manually.

'{(other sourcemap properties...), "mappings": "' + encode(decoded.mappings) + '"}'

I'm making the assumption that V8 would make this more memory efficient by not copying the encoded string into the JSON, but instead making the output JSON into a rope containing 3 strings.

},
set(value) {
Object.defineProperty(result, "map", {
value,
writable: true,
jridgewell marked this conversation as resolved.
Show resolved Hide resolved
});
}
});
result.decoded_map = options.format.source_map.getDecoded();
if (options.sourceMap.url == "inline") {
var sourceMap = typeof result.map === "object" ? JSON.stringify(result.map) : result.map;
result.code += "\n//# sourceMappingURL=data:application/json;charset=utf-8;base64," + to_base64(sourceMap);
Expand Down
71 changes: 50 additions & 21 deletions lib/sourcemap.js
View file
Open in desktop
Expand Up @@ -43,42 +43,47 @@

"use strict";

import MOZ_SourceMap from "source-map";
import {AnyMap, originalPositionFor} from "@jridgewell/trace-mapping";
import {defaults} from "./utils/index.js";
import {SourceMapConsumer, SourceMapGenerator} from "@jridgewell/source-map";
import {defaults, HOP} from "./utils/index.js";

// a small wrapper around source-map and @jridgewell/trace-mapping
function SourceMap(options) {
// a small wrapper around source-map and @jridgewell/source-map
async function SourceMap(options) {
options = defaults(options, {
file : null,
root : null,
orig : null,

orig_line_diff : 0,
dest_line_diff : 0,
files: {},
});

var orig_map;
var generator = new MOZ_SourceMap.SourceMapGenerator({
var generator = new SourceMapGenerator({
file : options.file,
sourceRoot : options.root
});

let sourcesContent = {__proto__: null};
let files = options.files;
for (var name in files) if (HOP(files, name)) {
sourcesContent[name] = files[name];
}
if (options.orig) {
orig_map = new AnyMap(options.orig);
// We support both @jridgewell/source-map (which has a sync
// SourceMapConsumer) and source-map (which has an async
// SourceMapConsumer).
orig_map = await new SourceMapConsumer(options.orig);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

if (orig_map.sourcesContent) {
orig_map.resolvedSources.forEach(function(source, i) {
var sourceContent = orig_map.sourcesContent[i];
if (sourceContent) {
generator.setSourceContent(source, sourceContent);
orig_map.sources.forEach(function(source, i) {
var content = orig_map.sourcesContent[i];
if (content) {
sourcesContent[source] = content;
}
});
}
}

function add(source, gen_line, gen_col, orig_line, orig_col, name) {
if (orig_map) {
var info = originalPositionFor(orig_map, {
var info = orig_map.originalPositionFor({
line: orig_line,
column: orig_col
});
Expand All @@ -91,18 +96,42 @@ function SourceMap(options) {
name = info.name || name;
}
generator.addMapping({
generated : { line: gen_line + options.dest_line_diff, column: gen_col },
original : { line: orig_line + options.orig_line_diff, column: orig_col },
generated : { line: gen_line, column: gen_col },
original : { line: orig_line, column: orig_col },
source : source,
name : name
});
generator.setSourceContent(source, sourcesContent[source]);
}

function clean(map) {
const allNull = map.sourcesContent && map.sourcesContent.every(c => c == null);
if (allNull) delete map.sourcesContent;
if (map.file === undefined) delete map.file;
if (map.sourceRoot === undefined) delete map.sourceRoot;
return map;
}

function getDecoded() {
if (!generator.toDecodedMap) return null;
return clean(generator.toDecodedMap());
}

function getEncoded() {
return clean(generator.toJSON());
}

function destroy() {
// @jridgewell/source-map's SourceMapConsumer does not need to be
// manually freed.
if (orig_map && orig_map.destroy) orig_map.destroy();
}

return {
add : add,
get : function() { return generator; },
toString : function() { return generator.toString(); },
destroy : function() {}
add,
getDecoded,
getEncoded,
destroy,
};
}

Expand Down
48 changes: 39 additions & 9 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Expand Up @@ -43,10 +43,9 @@
"main.js"
],
"dependencies": {
"@jridgewell/trace-mapping": "^0.3.5",
"@jridgewell/source-map": "^0.3.2",
"acorn": "^8.5.0",
"commander": "^2.20.0",
"source-map": "~0.8.0-beta.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tests still use this (namely test/mocha/sourcemaps.js) so it should go into devDependencies

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I didn't purge my node_modules. Fixed.

"source-map-support": "~0.5.20"
},
"devDependencies": {
Expand All @@ -59,7 +58,8 @@
"pre-commit": "^1.2.2",
"rimraf": "^3.0.2",
"rollup": "2.56.3",
"semver": "^7.3.4"
"semver": "^7.3.4",
"source-map": "~0.8.0-beta.0"
},
"scripts": {
"test": "node test/compress.js && mocha test/mocha",
Expand Down
2 changes: 1 addition & 1 deletion rollup.config.js
View file
Open in desktop
Expand Up @@ -5,7 +5,7 @@ export default () => {
file: "dist/bundle.min.js",
format: "umd",
globals: {
"source-map": "sourceMap",
"@jridgewell/source-map": "sourceMap",
},
name: "Terser",
sourcemap: false,
Expand Down
2 changes: 1 addition & 1 deletion test/input/issue-505/output.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion test/input/issue-520/output.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion test/input/source-maps/expect.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions test/mocha/cli-2.js
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@ describe("bin/terser (2)", function() {

assert.strictEqual(stdout, [
'"use strict";var foo=function foo(x){return"foo "+x};console.log(foo("bar"));',
"//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImluZGV4LmpzIl0sIm5hbWVzIjpbImZvbyIsIngiLCJjb25zb2xlIiwibG9nIl0sIm1hcHBpbmdzIjoiYUFBQSxJQUFJQSxJQUFNLFNBQU5BLElBQU1DLEdBQUEsTUFBSyxPQUFTQSxHQUN4QkMsUUFBUUMsSUFBSUgsSUFBSSJ9",
"//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJuYW1lcyI6WyJmb28iLCJ4IiwiY29uc29sZSIsImxvZyJdLCJzb3VyY2VzIjpbImluZGV4LmpzIl0sIm1hcHBpbmdzIjoiYUFBQSxJQUFJQSxJQUFNLFNBQU5BLElBQU1DLEdBQUEsTUFBSyxPQUFTQSxHQUN4QkMsUUFBUUMsSUFBSUgsSUFBSSJ9",
""
].join("\n"));
done();
Expand All @@ -46,7 +46,7 @@ describe("bin/terser (2)", function() {

assert.strictEqual(stdout, [
'function foo(){return function(){console.log("PASS")}}foo()();',
"//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbInRlc3QvaW5wdXQvaXNzdWUtMjMxMC9pbnB1dC5qcyJdLCJuYW1lcyI6WyJmb28iLCJjb25zb2xlIiwibG9nIiwiZiJdLCJtYXBwaW5ncyI6IkFBQUEsU0FBU0EsTUFDTCxPQUFPLFdBQ0hDLFFBQVFDLElBQUksU0FLUkYsS0FDUkcifQ==",
"//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJuYW1lcyI6WyJmb28iLCJjb25zb2xlIiwibG9nIiwiZiJdLCJzb3VyY2VzIjpbInRlc3QvaW5wdXQvaXNzdWUtMjMxMC9pbnB1dC5qcyJdLCJtYXBwaW5ncyI6IkFBQUEsU0FBU0EsTUFDTCxPQUFPLFdBQ0hDLFFBQVFDLElBQUksU0FLUkYsS0FDUkcifQ==",
""
].join("\n"));
done();
Expand Down
2 changes: 1 addition & 1 deletion test/mocha/minify.js
View file
Open in desktop
Expand Up @@ -321,7 +321,7 @@ describe("minify", function() {
});
var code = result.code;
assert.strictEqual(code, "var a=function(n){return n};\n" +
"//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIjAiXSwibmFtZXMiOlsiYSIsImZvbyJdLCJtYXBwaW5ncyI6IkFBQUEsSUFBSUEsRUFBSSxTQUFTQyxHQUFPLE9BQU9BIn0=");
"//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJuYW1lcyI6WyJhIiwiZm9vIl0sInNvdXJjZXMiOlsiMCJdLCJtYXBwaW5ncyI6IkFBQUEsSUFBSUEsRUFBSSxTQUFTQyxHQUFPLE9BQU9BIn0=");
});
it("should not append source map to output js when sourceMapInline is not enabled", async function() {
var result = await minify("var a = function(foo) { return foo; };");
Expand Down