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
Switch to GenMapping for sourcemap generation #1190
Changes from 1 commit
d23354e
04953b4
8f938df
f6dbb8f
b2e5a12
6cab7f2
2cc6270
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -271,18 +271,15 @@ async function minify(files, options, _fs_module) { | |
} | ||
if (!HOP(options.format, "code") || options.format.code) { | ||
if (options.sourceMap) { | ||
options.format.source_map = await SourceMap({ | ||
if (options.sourceMap.includeSources && files instanceof AST_Toplevel) { | ||
throw new Error("original source content unavailable"); | ||
} | ||
options.format.source_map = 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) { | ||
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; | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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.decodedMap = options.format.source_map.getDecoded(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decoded maps are free to generate from
jridgewell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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); | ||
|
@@ -310,9 +317,6 @@ async function minify(files, options, _fs_module) { | |
options.nameCache.props = cache_to_json(options.mangle.properties.cache); | ||
} | ||
} | ||
if (options.format && options.format.source_map) { | ||
options.format.source_map.destroy(); | ||
} | ||
if (timings) { | ||
timings.end = Date.now(); | ||
result.timings = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,34 +43,37 @@ | |
|
||
"use strict"; | ||
|
||
import MOZ_SourceMap from "source-map"; | ||
import {GenMapping, maybeAddMapping, toDecodedMap, toEncodedMap, setSourceContent} from "@jridgewell/gen-mapping"; | ||
import {AnyMap, originalPositionFor} from "@jridgewell/trace-mapping"; | ||
import {defaults} from "./utils/index.js"; | ||
import {defaults, HOP} from "./utils/index.js"; | ||
|
||
// a small wrapper around source-map and @jridgewell/trace-mapping | ||
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 GenMapping({ | ||
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); | ||
if (orig_map.sourcesContent) { | ||
orig_map.resolvedSources.forEach(function(source, i) { | ||
var sourceContent = orig_map.sourcesContent[i]; | ||
if (sourceContent) { | ||
generator.setSourceContent(source, sourceContent); | ||
var content = orig_map.sourcesContent[i]; | ||
if (content) { | ||
sourcesContent[source] = content; | ||
} | ||
}); | ||
} | ||
|
@@ -90,19 +93,27 @@ function SourceMap(options) { | |
orig_col = info.column; | ||
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 }, | ||
maybeAddMapping(generator, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is a change from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like less RAM usage to me, so this is a nice change! |
||
generated : { line: gen_line, column: gen_col }, | ||
original : { line: orig_line, column: orig_col }, | ||
source : source, | ||
name : name | ||
}); | ||
setSourceContent(generator, source, sourcesContent[source]); | ||
} | ||
|
||
function clean(map) { | ||
const allNull = 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; | ||
} | ||
|
||
return { | ||
add : add, | ||
get : function() { return generator; }, | ||
toString : function() { return generator.toString(); }, | ||
destroy : function() {} | ||
add : add, | ||
getDecoded : function() { return clean(toDecodedMap(generator)); }, | ||
getEncoded : function() { return clean(toEncodedMap(generator)); }, | ||
}; | ||
} | ||
|
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,10 +43,10 @@ | |
"main.js" | ||
], | ||
"dependencies": { | ||
"@jridgewell/gen-mapping": "^0.3.0", | ||
"@jridgewell/trace-mapping": "^0.3.5", | ||
"acorn": "^8.5.0", | ||
"commander": "^2.20.0", | ||
"source-map": "~0.8.0-beta.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I didn't purge my |
||
"source-map-support": "~0.5.20" | ||
}, | ||
"devDependencies": { | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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, callingsetSourceContent
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 thatfile.js
has contents, thenfile.js
will be in the output map even if it's not actually referenced by any mappings. By moving this intoSourceMap
, we can do a property lookup for source contents as they're needed.