Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Sourcemaps #141

Closed
wants to merge 5 commits into from

4 participants

@download13

To test, just ender build any library, create a test.html file that loads the ender.min.js file, and run it in Chrome (don't forget to turn on Source Maps in the Inspector options).
I haven't tested it with anything other than small and simple libraries yet, but it should work with any library that consists of one main file. I don't think whether or not it has an ender bridge should matter.

@rvagg
Owner

that's pretty neat, I'll have to find some time to have a look at it in more detail but it's great that you're on to this!

@download13

That'd be great. The main issue now is that I need to someone who knows the existing codebase to explain how I can get the filepath and contents info without breaking everything.

@rvagg
Owner

Just tried this out and it's awesome to see in action, great job so far!

@rvagg
Owner

It might be best to leave this unmerged for now because it's easier to discuss code in PR form. I might put a few suggestions in the diff.

lib/source-build.js
@@ -65,6 +68,10 @@ var async = require('async')
}).join(' ')
}
+ this.packages.forEach(function(v, i) {
+ this.sourcesMap[v.filename] = v.fileContents;
+ }, this);
+
@rvagg Owner
rvagg added a note

The main suggestion I have for this code as it is now is for you to try and move this functionality down into source-package.js and have source-build.js assemble the data upon request from its child package objects, in the same way that asString() does. So the roles are left clearly separated -- a SourceBuild object knows about the higher level build stuff, like what packages it has, in what order, and not a whole lot beyond that. In turn, its packages are a bunch of SourcePackage objects which know all the detail about individual packages/modules within that build, like what the file(s) are. And recall that a SourcePackage will often have mutliple files, a "main" and an "ender" and possibly multiples of each of those too, only the SourcePackage object knows about any of this.

So, look at the async.map() call in SourceBuild.asString() and you could do the same to implement a SourceBuild.sourceesMap() method (map the package list to the value of SourcePackage.asString() of each one). So you'd delegate down to the children SourcePackage objects what goes into that map, perhaps with some logic in SourceBuild to make sense of it before returning it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/write.js
((23 lines not shown))
, write = function (options, sourceBuild, out, callback) {
- async.parallel([
- writePlainFile.bind(null, options, sourceBuild, out)
- , writeMinifiedFile.bind(null, options, sourceBuild, out)
- ], callback)
+ sourceBuild.asString({ type: 'plain' }, function(err, source) {
+ var filename = util.getOutputFilenameFromOptions(options)
+ var nonMinSource = source;
+
+ writeFile(filename, source, function() {
+ var filename = util.getOutputFilenameFromOptions(options).replace(/(\.min)?\.js/, '.min.js')
+ sourceBuild.asString({ type: 'minified' }, function(err, source, sourceMapSeg2) {
+ var sourcesMap = sourceBuild.sourcesMap;
@rvagg Owner
rvagg added a note

perhaps this is premature but I'd like to see this logic elsewhere, mainly because it's confusing the role of write.js, perhaps the SourceMap object can do more of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rvagg rvagg commented on the diff
lib/write.js
((7 lines not shown))
fs.writeFile(file, data, 'utf-8', function (err) {
if (err) return callback(new FilesystemError(err))
callback.apply(null, arguments)
})
}
- , writePlainFile = function (options, sourceBuild, out, callback) {
- var filename = util.getOutputFilenameFromOptions(options)
- sourceBuild.asString({ type: 'plain' }, writeFile.bind(null, filename, callback))
- }
-
- , writeMinifiedFile = function (options, sourceBuild, out, callback) {
- var filename = util.getOutputFilenameFromOptions(options).replace(/(\.min)?\.js/, '.min.js')
- sourceBuild.asString({ type: 'minified' }, writeFile.bind(null, filename, callback))
@rvagg Owner
rvagg added a note

As a side note, you mentioned in #127 about the use of .bind() all over the place: you'll find that its main use is not to assign this but to curry additional arguments to functions that need to be passed around, so we don't end up with callbacks within callbacks and it's easier to organise. For instance in this above line we're simply passing in the writeFile function as the callback argument to SourceBuild.asString() but we're injecting the first 2 arguments, filename and callback so they are preserved and SourceBuild.asString() is oblivious to them, plus we end up with smaller, more narrowly focused functions. It's the same as using async.apply() (which you'll find a lot in the old CLI code).

There's only a few actual instantiatable objects (in the traditional sense) in the code: SourceBuild, SourcePackage and PackageDescriptor (but that one will likely change) so they are the only places you'll find a .bind(this). They also use Object.create() in preference to a classical prototypal setup with a constructor and the use of new.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
resources/source-package.mustache
((6 lines not shown))
provide("{{packageName}}", module.exports);
{{#options.sandbox}}
window["{{packageName}}"] = module.exports;
{{/options.sandbox}}{{#enderSource}}
-{{{indented}}}
+{{{raw}}}
@rvagg Owner
rvagg added a note

One problem we have is that we leave it up to the templates to decide whether to indent or not (which you've obviously discovered!). I think perhaps the best way to deal with this is keep track of which was requested by the templates. This can be done here, that information can then be used in building the source map data. Then the templates are free to indent or use the raw (but not both!).

The information about how many indent characters are used will need to be available. The indent function in source-package.js holds the information about how many characters the code will be indented. Should that be changed to an exported global so we can see it elsewhere, or is there a better way to handle it?

@rvagg Owner
rvagg added a note

well this is another reason that putting some of the dirty source map logic into source-package.js makes sense, because so much of what it needs to know is in there, including the indent character count. SourceBuild asks all its SourcePackages for source map data for each module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rvagg
Owner

Heads-up on some new changes pushed to 1.0-wip that change source-package.js (amongst other things). Glob matching is now allowed in "ender" and "main" file lists, at the moment multiple files are simply appended together (see source-package-util.js). By far the most common case will be a single file for "main" and/or "ender" so for now I'm happy for the source maps code to assume there is a single file and we can make it deal with multiple files before release.

@download13

I've just finished a few more changes (though still with some inexplicable bugs, there must be a better way to look at giant tables of numbers and see where you've gone wrong), so I'll try to merge them with the new code tomorrow.

@download13

Actually, I should probably ask how to feel about changing something that's kind of important to the .sources() function I've tacked onto the SourcePackage and SourceBuild objects. Since it needs access to file path and contents information from the loadFilesAsString function (on a per-file basis as opposed to concatenated together), I ended up renaming it the loadFiles function and making it return {file: '/file/path', content: 'source here'} objects instead. The processing for the template is done in the handler callback within asSource.
The only alternative I could see is to write a whole separate function that does essentially the same thing but returns the information needed to work with sources. Do you have any thoughts on the matter?

@rvagg
Owner

I'm fine with that. That function has been moved to source-package-util.js in the newest revision. In the future we'll be wanting to handle the files differently anyway, not just concatenating.

You'll see a new function: collectFiles() that takes individual elements of the original array (from package.json, both "main" and "ender", and non-arrays are turned into single-element arrays so it always handles arrays) and expands them using node-glob, if it's simply a filename then it'll come back with that same filename but if it's a glob ("*.js" for example) then it'll come back with a list of files that match that glob. So what you'll want to do is preserve those names through the async.map() in loadFilesAsString() (or whatever you rename it to) and pass them out like you've suggested.

I look forward to seeing what you have! I've had a basic play in Chrome with a build from your branch and it's very exciting!

@rvagg
Owner

actually... I've gone ahead and done this for you since I know we're going to need this, regardless of source map support.

d78bd38

You probably can ignore source-package-util.js completely now, from loadFiles() you'll get an array of 'filename' & 'contents' pairs, see the tests for examples. Right now in source-package.js we just pull out the contents and join them together. You can do what you need with this data cause you have the full filename (including path) plus the real contents of the file; in the order they will be used (separated by \n\n still).

@download13

I've merged the new changes into the sourcemap branch.

Since I couldn't find a cause for the strange issue in Chrome where breakpoints set on one line would jump to another, I ended up testing it by just running closure compiler on a small JS file and opening it with a map in Chrome.
The results: Yay, it's not my fault! On a less yay note, there seems to be something wrong with either closure compiler or Chrome's handling of source maps which means we can't test the maps reliably.

It's basically working now, but I haven't refactored the SourceMap object to use the layout style of the rest of the code since I can't test it yet. The changes are at a432e2e if you want to look at them.

@rvagg
Owner

Will check it out as soon as I have time. In the meantime you should file a bug with the Chrome guys. Source maps support being so new they'd probably appreciate hearing about issues like this. http://crbug.com/new
I've already filed one asking for a link between stack trace file references and the source mapped versions.

@ded
Owner

i think it's time to revisit source maps again with Ender. we should definitely move this to the top of the list and eventually cut a 1.1

@amccollum
Owner

This is now released in ender@2.0.x.

@amccollum amccollum closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 4, 2012
  1. @download13
Commits on May 5, 2012
  1. @download13
Commits on May 13, 2012
  1. @download13
Commits on May 14, 2012
  1. @download13
  2. @download13
This page is out of date. Refresh to see the latest.
View
30 lib/minify-closure.js
@@ -0,0 +1,30 @@
+var path = require('path')
+var fs = require('fs')
+var child_process = require('child_process')
+
+var supportpath = path.resolve(__dirname, '../support/')
+var jarpath = path.join(supportpath, 'closure.jar')
+var mappath = '_tmp_map' // Should probably make use of that generateTempFile function
+var cmd = 'java -jar "' + jarpath + '" --compilation_level SIMPLE_OPTIMIZATIONS --create_source_map ' + mappath + ' --source_map_format V3'
+
+// This needs some error handling and general cleanup
+function minify(source, cb) {
+ var child = child_process.exec(cmd), minified = ''
+
+ child.stdout.setEncoding('utf8')
+ child.stdout.on('data', function(data) {
+ minified += data
+ })
+
+ child.on('exit', function() {
+ fs.readFile(mappath, 'utf8', function(err, mapdata) {
+ fs.unlink(mappath, function () {
+ cb(null, minified, mapdata)
+ })
+ })
+ })
+
+ child.stdin.end(source);
+}
+
+exports.minify = minify;
View
26 lib/source-build.js
@@ -32,7 +32,7 @@
var async = require('async')
, fs = require('fs')
- , minify = require('./minify')
+ , minify = require('./minify-closure')
, template = require('./template')
, argsParse = require('./args-parse')
, BuildParseError = require('./errors').BuildParseError
@@ -46,6 +46,10 @@ var async = require('async')
init: function (options) {
this.options = options
this.packages = []
+
+ this.asString = this.asString.bind(this) // Are these needed?
+ this.sources = this.sources.bind(this)
+
return this
}
@@ -84,6 +88,26 @@ var async = require('async')
}
)
}
+
+ , sources: function (callback) {
+ async.map(
+ this.packages
+ , function (srcPackage, callback) {
+ srcPackage.sources(callback)
+ }
+ , function (err, sources) {
+ if (err) return callback(err)
+
+ var glom = {}
+ sources.forEach(function (v) {
+ for(var file in v) {
+ glom[file] = v[file]
+ }
+ })
+ callback(null, glom)
+ }
+ )
+ }
}
// a utility static method to partially read an ender build file and parse the head comment
View
3  lib/source-package-util.js
@@ -54,9 +54,10 @@ var fs = require('fs')
}
// utility to read multiple files in order and append them
- , loadFiles = function (root, files, callback) {
+ , loadFiles = function (files, callback) {
if (!Array.isArray(files)) files = [ files ]
if (!files.length || (files.length == 1 && files[0] == 'noop')) return callback()
+ var root = this.rootPath
collectFiles(root, files, function (err, files) {
// read each source file in parallel and assemble them together
View
35 lib/source-package.js
@@ -49,6 +49,7 @@ var fs = require('fs')
template.generateSource(templateFiles[root ? 'root' : 'standard'], data, callback)
}
+ , INDENT_STR = ' '
, indent = function (str) {
// was this: return str.replace(/^(?!\s*$)/gm, ' ')
// but in some odd cases ^ was matching other things and inserting ' ' in unhelpful places
@@ -58,7 +59,7 @@ var fs = require('fs')
// which starts with: // If the space parameter...
// and gets converted to: / / If the space parameter
return str.split('\n').map(function (line) {
- return (/^\s*$/).test(line) ? line : (' ' + line)
+ return (/^\s*$/).test(line) ? line : (INDENT_STR + line)
}).join('\n')
}
@@ -94,10 +95,12 @@ var fs = require('fs')
this.isRoot = isRoot
this.packageName = packageName
this.packageJSON = packageJSON
+ this.rootPath = packageUtil.getPackageRoot(this.parents, this.packageName) // Moved to init since it gets used more than once
// custom hasher function for async.memoize so we have a single key, default will use
// first arg (callback) as hash key which won't work
this.asString = async.memoize(this.asString.bind(this), function () { return '_' })
+ this.sources = this.sources.bind(this)
return this
}
@@ -110,8 +113,7 @@ var fs = require('fs')
// note that "main" and "ender" are processed in the same way so they can both be just
// a string pointing to a source file or an array of source files that are concatenated
// or be left unspecified
- var root = packageUtil.getPackageRoot(this.parents, this.packageName)
- , mainSources = this.packageJSON.main || []
+ var mainSources = this.packageJSON.main || []
, enderBridgeSources = this.packageJSON.ender || []
, handleSourceData = function (err, sources) {
@@ -132,14 +134,35 @@ var fs = require('fs')
}.bind(this)
, sourceLoaders = {
- main: sourcePackageUtil.loadFiles.bind(this, root, mainSources)
- , ender: sourcePackageUtil.loadFiles.bind(this, root, enderBridgeSources)
+ main: sourcePackageUtil.loadFiles.bind(this, mainSources)
+ , ender: sourcePackageUtil.loadFiles.bind(this, enderBridgeSources)
}
async.parallel(sourceLoaders, handleSourceData)
}
+
+ , sources: function (callback) {
+ async.parallel({
+ main: sourcePackageUtil.loadFiles.bind(this, this.packageJSON.main || [])
+ , ender: sourcePackageUtil.loadFiles.bind(this, this.packageJSON.ender || [])
+ }, function (err, sources) {
+ if (err) return callback(err)
+
+ var map = {}
+ function toMap(source) {
+ map[path.relative('.', source.file)] = source.contents
+ }
+
+ sources.main && sources.main.forEach(toMap)
+ sources.ender && sources.ender.forEach(toMap)
+
+ callback(err, map)
+ }.bind(this))
+ }
}
module.exports.create = function (packageName, parents, isRoot, packageJSON, options) {
return Object.create(SourcePackage).init(packageName, parents, isRoot, packageJSON, options)
-}
+}
+exports.indent = indent;
+exports.INDENT_STR = INDENT_STR;
View
180 lib/sourcemap.js
@@ -0,0 +1,180 @@
+var SourcePackage = require('./source-package');
+
+function SourceMap(data) {
+ data = JSON.parse(data);
+ this.sources = data.sources;
+ this.names = data.names;
+ this.version = data.version;
+ this.sourceRoot = data.sourceRoot;
+ this.file = data.file;
+
+ var prev = [0, 0, 0, 0, 0];
+ this.groups = data.mappings.split(';').map(function(group) {
+ prev[0] = 0;
+ return group.split(',').map(function(segment) {
+ segment = decodeVLQStr(segment);
+
+ segment = segment.map(function(v, i) { // Stabby function would really help here
+ return v + prev[i];
+ });
+ prev = prev.map(function(v, i) {
+ v = segment[i];
+ return (v != null && !isNaN(v)) ? v : prev[i];
+ });
+
+ var r = {};
+ for(var i = 0; i < segment.length; i++) {
+ r['field' + (i + 1)] = segment[i];
+ }
+ return r;
+ });
+ });
+}
+SourceMap.buildSourceMap = function(options, plainSource, sources, orgMap) {
+ var map = {}
+ for(var filepath in sources) {
+ var source = sources[filepath], cols = 0
+ if (!options.noop) { // This may not work if the template changes. Solutions?
+ source = SourcePackage.indent(source)
+ cols = SourcePackage.INDENT_STR.length
+ }
+
+ var start = plainSource.indexOf(source)
+ var offset = plainSource.substr(0, start).split('\n').length - 1
+ map[filepath] = {offsetTop: offset, offsetLeft: cols, lines: source.split('\n').length}
+ }
+
+ var sm = new SourceMap(orgMap)
+ sm.translate(map)
+ return sm.serialize()
+}
+SourceMap.prototype = {
+ translate: function(offsetMap) {
+ var sources = this.sources = Object.keys(offsetMap);
+ var groups = this.groups;
+ var self = this;
+
+ sources.forEach(function(sourceName, sourceIndex) {
+ var source = offsetMap[sourceName];
+ var offsetTop = source.offsetTop;
+ var offsetLeft = source.offsetLeft;
+ var lines = source.lines;
+
+ groups.forEach(function(segments, minline) {
+ segments.forEach(function(segment) {
+ if(segment.field3 == null) {
+ return;
+ }
+ var tmp = segment.field3 - offsetTop;
+ if(tmp >= 0 && tmp < lines) {
+ //console.log(sourceName, offsetTop, lines, tmp + ' <- ' + segment.field3, segment.field4 - offsetLeft + ' <- ' + segment.field4, (self.names[segment.field5] || segment.field5));
+ segment.field3 = tmp;
+ segment.field2 = sourceIndex;
+ segment.field4 -= offsetLeft;
+ }
+ });
+ });
+ });
+ },
+ serialize: function() {
+ var prev = [0, 0, 0, 0, 0];
+ var mappings = this.groups.map(function(group) {
+ prev[0] = 0;
+ return group.map(function(segment) {
+ var r = [];
+ var len = Object.keys(segment).length;
+ for(var i = 0; i < len; i++) {
+ r.push(segment['field' + (i + 1)]);
+ }
+ segment = r;
+
+ var saved = segment;
+ segment = segment.map(function(v, i) {
+ return v - prev[i];
+ });
+ prev = saved.map(function(v, i) {
+ return (v != null && !isNaN(v)) ? v : prev[i];
+ });
+
+ return encodeVLQArr(segment);
+ }).join(',');
+ }).join(';');
+
+ return JSON.stringify({
+ version: this.version,
+ file: this.file,
+ sourceRoot: this.sourceRoot,
+ sources: this.sources,
+ names: this.names,
+ mappings: mappings
+ });
+ }
+}
+
+
+var iToB = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/'.split('');
+var bToI = {};
+iToB.forEach(function(v, i) {
+ bToI[v] = i;
+});
+
+function encodeVLQArr(values) {
+ return values.map(function(v) {
+ return encodeVLQ(v);
+ }).join('');
+}
+function encodeVLQ(aValue) {
+ var encoded = '';
+ var vlq = toVLQSigned(aValue);
+ var sign = aValue < 0 ? 1 : 0;
+
+ do {
+ var digit = vlq & 31; // Mask to first 5 bits
+ vlq >>>= 5; // Remove the first 5 bits
+ if(vlq > 0) {
+ digit |= 32; // Set sixth bit
+ }
+ encoded += iToB[digit];
+ } while(vlq > 0);
+
+ return encoded;
+}
+function toVLQSigned(aValue) {
+ return aValue < 0 ? ((-aValue) << 1) | 1 : (aValue << 1);
+}
+
+function decodeVLQStr(str) {
+ var r = [];
+ while(str.length > 0) {
+ var t = decodeVLQ(str);
+ r.push(t.value);
+ str = t.rest;
+ }
+ return r;
+}
+function decodeVLQ(aStr) {
+ var i = 0;
+ var result = 0;
+ var shift = 0;
+ var continuation;
+
+ do {
+ if(i >= aStr.length) {
+ throw new Error('Expected more digits in base 64 VLQ value.');
+ }
+
+ var digit = bToI[aStr[i++]];
+ continuation = digit >>> 5;
+ digit &= 31;
+ result |= digit << shift;
+ shift += 5;
+ } while(continuation);
+
+ return {value: fromVLQSigned(result), rest: aStr.slice(i)};
+}
+function fromVLQSigned(aValue) {
+ var shifted = aValue >>> 1;
+ return (aValue & 1) ? -shifted : shifted;
+}
+
+module.exports = SourceMap;
View
40 lib/write.js
@@ -34,30 +34,38 @@ var fs = require('fs')
, async = require('async')
, util = require('./util')
, FilesystemError = require('./errors').FilesystemError
+ , SourceMap = require('./sourcemap')
- , writeFile = function (file, callback, err, data) {
- if (err) return callback(err) // wrapped in source-build.js
+ , writeFile = function (file, data, callback) {
fs.writeFile(file, data, 'utf-8', function (err) {
if (err) return callback(new FilesystemError(err))
callback.apply(null, arguments)
})
}
- , writePlainFile = function (options, sourceBuild, out, callback) {
- var filename = util.getOutputFilenameFromOptions(options)
- sourceBuild.asString({ type: 'plain' }, writeFile.bind(null, filename, callback))
- }
-
- , writeMinifiedFile = function (options, sourceBuild, out, callback) {
- var filename = util.getOutputFilenameFromOptions(options).replace(/(\.min)?\.js/, '.min.js')
- sourceBuild.asString({ type: 'minified' }, writeFile.bind(null, filename, callback))
@rvagg Owner
rvagg added a note

As a side note, you mentioned in #127 about the use of .bind() all over the place: you'll find that its main use is not to assign this but to curry additional arguments to functions that need to be passed around, so we don't end up with callbacks within callbacks and it's easier to organise. For instance in this above line we're simply passing in the writeFile function as the callback argument to SourceBuild.asString() but we're injecting the first 2 arguments, filename and callback so they are preserved and SourceBuild.asString() is oblivious to them, plus we end up with smaller, more narrowly focused functions. It's the same as using async.apply() (which you'll find a lot in the old CLI code).

There's only a few actual instantiatable objects (in the traditional sense) in the code: SourceBuild, SourcePackage and PackageDescriptor (but that one will likely change) so they are the only places you'll find a .bind(this). They also use Object.create() in preference to a classical prototypal setup with a constructor and the use of new.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- }
-
, write = function (options, sourceBuild, out, callback) {
- async.parallel([
- writePlainFile.bind(null, options, sourceBuild, out)
- , writeMinifiedFile.bind(null, options, sourceBuild, out)
- ], callback)
+ var plainFilename = util.getOutputFilenameFromOptions(options), sourceMap
+ var mapFilename = plainFilename.replace(/(\.min)?\.js/, '.map.js')
+ // These can't really be separate since now the write stages depend on one-another
+ async.parallel({
+ plainSource: sourceBuild.asString.bind(null, {type: 'plain'})
+ , miniSource: function (callback) {
+ sourceBuild.asString({type: 'minified'}, function (err, minified, map) { // Is there a better way to get the source map from the minifier?
+ sourceMap = map
+ callback(err, minified)
+ })
+ }
+ , sources: sourceBuild.sources
+ }, function (err, results) {
+ sourceMap = SourceMap.buildSourceMap(options, results.plainSource, results.sources, sourceMap)
+ var miniSource = results.miniSource + '//@ sourceMappingURL=' + mapFilename; // We might want an option for this too
+
+ async.parallel([
+ writeFile.bind(null, plainFilename, results.plainSource)
+ , writeFile.bind(null, plainFilename.replace(/(\.min)?\.js/, '.min.js'), miniSource)
+ , writeFile.bind(null, mapFilename, sourceMap)
+ ], callback)
+ })
}
module.exports.write = write
Something went wrong with that request. Please try again.