From 013c9f82e7a27ae723d0765d794807854e832791 Mon Sep 17 00:00:00 2001 From: Geoff Schmidt Date: Thu, 13 Jun 2013 16:58:43 -0700 Subject: [PATCH 01/49] groundwork --- tools/bundler.js | 11 +++++++++++ tools/linker.js | 15 ++++++++++++--- tools/packages.js | 11 +++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/tools/bundler.js b/tools/bundler.js index b2a5deff97..17802c0dd8 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -115,6 +115,17 @@ // be search for npm modules // - staticDirectory: directory to search for static assets when // Assets.getText and Assets.getBinary are called from this file. +// - sourceMap: if present, path of a file that contains a source +// map for this file, relative to program.json +// - sources: if sourceMap present, a map from a a relative path in +// the source map (no leading slash) to information about the +// source file: +// - source: path of this source file if available, relative to +// program.json +// - package: name of the package from which this file came, if +// any (omit if file came from an app) +// - sourcePath: original relative path within the source tree of +// 'package' (or the app) of this source file // // /config.json: // diff --git a/tools/linker.js b/tools/linker.js index 5cddc8b6e6..ff8ca2097a 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -110,7 +110,9 @@ _.extend(Module.prototype, { return globalReferences; }, - // Output is a list of objects with keys 'source' and 'servePath'. + // Output is a list of objects with keys 'source', 'servePath', + // 'sourceMap', 'sources' (map from relative path in source map to + // 'package', 'sourcePath', 'path') getLinkedFiles: function () { var self = this; @@ -132,7 +134,9 @@ _.extend(Module.prototype, { return { source: file.getLinkedOutput({ preserveLineNumbers: true, exports: moduleExports }), - servePath: file.servePath + servePath: file.servePath, + sourceMap: null, // XXX XXX + sources: {} // XXX XXX }; })); } @@ -168,7 +172,9 @@ _.extend(Module.prototype, { return [{ source: combined, - servePath: self.combinedServePath + servePath: self.combinedServePath, + sourceMap: null, // XXX XXX + sources: {} // XXX XXX }]; }, @@ -628,6 +634,9 @@ _.extend(Unit.prototype, { // // Output is an object with keys: // - files: is an array of output files in the same format as inputFiles +// - EXCEPT THAT, for now, sourcePath is omitted and is replaced with +// sourceMap (a SourceMapGenerator) and sources (map to keys 'package', +// 'sourcePath', 'path') similar to self.resources in a Slice (XXX) // - exports: the exports, as a list of string ('Foo', 'Thing.Stuff', etc) // - boundary: an opaque value that must be passed along with 'files' to link() var prelink = function (options) { diff --git a/tools/packages.js b/tools/packages.js index b1c4a3840d..ed8d079084 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -173,6 +173,15 @@ var Slice = function (pkg, options) { // honored for "static", ignored for "head" and "body", sometimes // honored for CSS but ignored if we are concatenating. // + // sourceMap: Allowed only for "js". If present, a + // SourceMapGenerator. + // + // sources: Only if 'sourceMap' present. A mapping from a relative + // source path given in sourceMap (no leading '/') to: + // - package: package name, or null for app + // - sourcePath: original relative path within 'package' or app + // - path: absolute path on disk to the source file + // // Set only when isBuilt is true. self.resources = null; @@ -541,6 +550,8 @@ _.extend(Slice.prototype, { data: new Buffer(file.source, 'utf8'), servePath: file.servePath, staticDirectory: self.staticDirectory + sourceMap: file.sourceMap, + sources: file.sources }; }); From 2a0f7afa7d92055b827a875ca8ca7b32a31bab56 Mon Sep 17 00:00:00 2001 From: Geoff Schmidt Date: Thu, 13 Jun 2013 20:48:27 -0700 Subject: [PATCH 02/49] Add 'source-map' to dev_bundle and bump dev_bundle version. (Consider this a preliminary version number, subject to change, until this branch is actually merged.) --- scripts/generate-dev-bundle.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/generate-dev-bundle.sh b/scripts/generate-dev-bundle.sh index ab37cea852..cca59e5309 100755 --- a/scripts/generate-dev-bundle.sh +++ b/scripts/generate-dev-bundle.sh @@ -109,6 +109,7 @@ npm install tar@0.1.14 npm install kexec@0.1.1 npm install shell-quote@0.0.1 npm install byline@2.0.3 +npm install source-map@0.1.22 # uglify-js has a bug which drops 'undefined' in arrays: # https://github.com/mishoo/UglifyJS2/pull/97 From 685a4ffdcf1a926c1fe8c7c87dc48c7bcf5a6304 Mon Sep 17 00:00:00 2001 From: Geoff Schmidt Date: Thu, 13 Jun 2013 20:51:17 -0700 Subject: [PATCH 03/49] Source maps are generated, but they (1) are only generated for server targets, not browser targets; (2) aren't used for anything, (3) can only be generated by linker (not by, say, coffeescript); (4) are stored incorrectly (inline in control files, rather than as separate files); (5) are wrong anyway (at minimum, they need to be adjusted for boundary string replacement during link) But hey. Source maps. --- tools/bundler.js | 61 ++++++++++++++++-- tools/linker.js | 161 +++++++++++++++++++++++++++++++++------------- tools/packages.js | 30 +++++++-- 3 files changed, 196 insertions(+), 56 deletions(-) diff --git a/tools/bundler.js b/tools/bundler.js index 17802c0dd8..675b3a4afa 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -242,19 +242,34 @@ var StaticDirectory = function (options) { // Allowed options: // - sourcePath: path to file on disk that will provide our contents // - data: contents of the file as a Buffer +// - sourceMap: if 'data' is given, can be given instead of sourcePath. a string +// - sources: if sourceMap is provided, map from relative path to object +// with keys 'package', 'sourcePath', 'source' // - cacheable var File = function (options) { var self = this; - if (options.data && ! (options.data instanceof Buffer)) { + if (options.data && ! (options.data instanceof Buffer)) throw new Error('File contents must be provided as a Buffer'); - } + if (! options.sourcePath && ! options.data) + throw new Error("Must provide either sourcePath or data"); // The absolute path in the filesystem from which we loaded (or will // load) this file (null if the file does not correspond to one on // disk.) self.sourcePath = options.sourcePath; + // If this file was generated, a sourceMap (provided as a string) + // with debugging information. Set with setSourceMap. + self.sourceMap = null; + + // If sourceMap is set, this is a map from a relative source file + // path in the source map (no leading '/') to an object with keys: + // - package: name of the source package, or null for the app + // - sourcePath: the source path, relative to the root of 'package' + // - source: full contents of the source file, as a Buffer + self.sources = null; + // Where this file is intended to reside within the target's // filesystem. self.targetPath = null; @@ -293,8 +308,9 @@ _.extend(File.prototype, { contents: function (encoding) { var self = this; if (! self._contents) { - if (! self.sourcePath) + if (! self.sourcePath) { throw new Error("Have neither contents nor sourcePath for file"); + } else self._contents = fs.readFileSync(self.sourcePath); } @@ -371,6 +387,17 @@ _.extend(File.prototype, { sourcePath: staticSourceDirectory, bundlePath: bundlePath }); + }, + + // Set a source map for this File. sourceMap is given as a + // string. See self.sources for the format of sources. + setSourceMap: function (sourceMap, sources) { + var self = this; + + if (typeof sourceMap !== "string") + throw new Error("sourceMap must be given as a string"); + self.sourceMap = sourceMap; + self.sources = sources || {}; } }); @@ -453,7 +480,7 @@ _.extend(Target.prototype, { self.minifyCss(); } - // Process asset directories (eg, /public) + // Process asset directories (eg, '/public') // XXX this should probably be part of the appDir reader _.each(options.assetDirs || [], function (ad) { self.addAssetDir(ad); @@ -599,7 +626,7 @@ _.extend(Target.prototype, { var isApp = ! slice.pkg.name; // Emit the resources - _.each(slice.getResources(self.arch), function (resource) { + _.each(slice.getResources(self.arch), function (resource) { if (_.contains(["js", "css", "static"], resource.type)) { if (resource.type === "css" && ! isBrowser) // XXX might be nice to throw an error here, but then we'd @@ -645,6 +672,11 @@ _.extend(Target.prototype, { f.nodeModulesDirectory = nmd; } + if (resource.type === "js" && resource.sourceMap) { + f.setSourceMap(resource.sourceMap.toString(), + resource.sources); + } + self[resource.type].push(f); return; } @@ -910,6 +942,9 @@ var JsImage = function () { // - source: JS source code to load, as a string // - nodeModulesDirectory: a NodeModulesDirectory indicating which // directory should be searched by Npm.require() + // - sourceMap: if set, source map for this code, AS A STRING + // - sources: map from relative path in source map to object with + // keys 'source' (a string), 'package', 'sourcePath' // note: this can't be called `load` at it would shadow `load()` self.jsToLoad = []; @@ -1013,8 +1048,8 @@ _.extend(JsImage.prototype, { }, bindings || {}); try { - // XXX Get the actual source file path -- item.targetPath is - // not actually correct (it's the path in the bundle rather + // XXX XXX Get the actual source file path -- item.targetPath + // is not actually correct (it's the path in the bundle rather // than in the source tree.) Moreover, we need to do source // mapping. files.runJavaScript(item.source, item.targetPath, env); @@ -1063,6 +1098,14 @@ _.extend(JsImage.prototype, { item.nodeModulesDirectory.preferredBundlePath : undefined, staticDirectory: item.staticDirectory ? item.staticDirectory.bundlePath : undefined + sourceMap: item.sourceMap || undefined, // XXX XXX WRONG -- should be a file, not an inline string + sources: item.sourceMap ? _.map(item.sources || [], function (x) { + return { + source: x.source.toString('utf8'), // XXX XXX WRONG -- should be a file, not an inline string + package: x.package, + sourcePath: x.sourcePath + }; + }) : undefined }); }); @@ -1127,6 +1170,8 @@ JsImage.readFromDisk = function (controlFilePath) { staticDirectory: new StaticDirectory({ sourcePath: item.staticDirectory }) + // XXX XXX set 'sourceMap' (to a string) and 'sources' (keys + // 'package', 'sourcePath', 'source' as a string) }); }); @@ -1156,6 +1201,8 @@ _.extend(JsImageTarget.prototype, { source: file.contents().toString('utf8'), nodeModulesDirectory: file.nodeModulesDirectory, staticDirectory: file.staticDirectory + sourceMap: file.sourceMap, + sources: file.sources }); }); diff --git a/tools/linker.js b/tools/linker.js index ff8ca2097a..7c4f07b0d8 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -1,5 +1,6 @@ var fs = require('fs'); var _ = require('underscore'); +var sourcemap = require('source-map'); var buildmessage = require('./buildmessage'); var packageDot = function (name) { @@ -112,7 +113,7 @@ _.extend(Module.prototype, { // Output is a list of objects with keys 'source', 'servePath', // 'sourceMap', 'sources' (map from relative path in source map to - // 'package', 'sourcePath', 'path') + // 'package', 'sourcePath', 'source') getLinkedFiles: function () { var self = this; @@ -131,12 +132,20 @@ _.extend(Module.prototype, { }]; return ret.concat(_.map(self.files, function (file) { + var node = file.getLinkedOutput({ preserveLineNumbers: true, + exports: moduleExports }), + var results = node.toStringWithSourceMap({ + file: file.servePath + }); // results has 'code' and 'map' attributes + + sources = {}; + file.addToSourcesSet(sources); + return { - source: file.getLinkedOutput({ preserveLineNumbers: true, - exports: moduleExports }), + source: results.code, servePath: file.servePath, - sourceMap: null, // XXX XXX - sources: {} // XXX XXX + sourceMap: results.map, + sources: sources }; })); } @@ -151,30 +160,37 @@ _.extend(Module.prototype, { var moduleScopedVars = self.computeModuleScopedVars(); // Prologue - var combined = "(function () {\n\n"; - combined += self.boundary; + var chunks = []; + chunks.push("(function () {\n\n" + self.boundary); if (moduleScopedVars.length) { - combined += "/* Package-scope variables */\n"; - combined += "var " + moduleScopedVars.join(', ') + ";\n\n"; + chunks.push("/* Package-scope variables */\n"); + chunks.push("var " + moduleScopedVars.join(', ') + ";\n\n"); } // Emit each file + var sources = {}; _.each(self.files, function (file) { - combined += file.getLinkedOutput({ sourceWidth: sourceWidth, - exports: moduleExports }); - combined += "\n"; + chunks.push(file.getLinkedOutput({ sourceWidth: sourceWidth, + exports: moduleExports })); + chunks.push("\n"); + file.addToSourcesSet(sources); }); // Epilogue - combined += self.getExportCode(); - combined += "\n})();"; + chunks.push(self.getExportCode()); + chunks.push("\n})();"); + + var node = new sourcemap.SourceNode(null, null, null, chunks); + var results = node.toStringWithSourceMap({ + file: self.combinedServePath + }); // results has 'code' and 'map' attributes return [{ - source: combined, + source: results.code, servePath: self.combinedServePath, - sourceMap: null, // XXX XXX - sources: {} // XXX XXX + sourceMap: results.map, + sources: sources }]; }, @@ -291,9 +307,14 @@ var File = function (inputFile, module) { // the path where this file would prefer to be served if possible self.servePath = inputFile.servePath; - // the path to use for error message + // The relative path of this input file in its source tree (eg, + // package or app.) Used for source maps, error messages.. self.sourcePath = inputFile.sourcePath; + // The source tree to which sourcePath is relative. Either a name of + // a package, or null to mean "the app". + self.package = null; // XXX XXX actually set this + // should line and column be included in errors? self.includePositionInErrors = inputFile.includePositionInErrors; @@ -331,32 +352,60 @@ _.extend(File.prototype, { return globalReferences; }, + // Relative path to use in source maps to indicate this file. No + // leading slash. + _pathForSourceMap: function () { + var self = this; + + if (self.package) + return "package/" + self.package + "/" + self.sourcePath; + else + return "app/" + self.sourcePath; + }, + + // Add to a 'sources' set (a map from source map relative paths to + // info about each source file) + addToSourcesSet: function (sources) { + var self = this; + sources[self._pathForSourceMap()] = { + package: self.package, + sourcePath: self.sourcePath, + source: new Buffer(self.source, 'utf8') // XXX encoding + }; + }, + // Options: // - preserveLineNumbers: if true, decorate minimally so that line // numbers don't change between input and output. In this case, // sourceWidth is ignored. // - sourceWidth: width in columns to use for the source code // - exports: the module's exports + // + // Returns a SourceNode. getLinkedOutput: function (options) { var self = this; - // XXX XXX if a unit is not going to be used, prepend each line with '//' - // The newline after the source closes a '//' comment. if (options.preserveLineNumbers) { // Ugly version + // XXX XXX need to propagate source maps through linkerUnitTransform! var body = self.linkerUnitTransform(self.source, options.exports); - if (body.length && body[body.length - 1] !== '\n') - body += '\n'; - return self.bare ? body : ("(function(){" + body + "})();\n"); + return new sourcemap.SourceNode(null, null, null, [ + self.bare ? "" : "(function(){", + new sourcemap.SourceNode(1, 0, self._pathForSourceMap(), + body), + body.length && body[body.length - 1] !== '\n' ? "\n" : "", + self.bare ? "" : "\n})();\n" + ]); } // Pretty version - var buf = ""; + var chunks = []; + var header = ""; // Prologue - if (!self.bare) - buf += "(function () {\n\n"; + if (! self.bare) + header += "(function () {\n\n"; // Banner var width = options.sourceWidth || 70; @@ -365,14 +414,16 @@ _.extend(File.prototype, { var spacer = "// " + new Array(bannerWidth - 6 + 1).join(' ') + " //\n"; var padding = new Array(bannerWidth + 1).join(' '); var blankLine = new Array(width + 1).join(' ') + " //\n"; - buf += divider + spacer; - buf += "// " + (self.servePath.slice(1) + padding).slice(0, bannerWidth - 6) + - " //\n"; + header += divider + spacer; + header += "// " + + (self.servePath.slice(1) + padding).slice(0, bannerWidth - 6) + " //\n"; if (self.bare) { var bareText = "This file is in bare mode and is not in its own closure."; - buf += "// " + (bareText + padding).slice(0, bannerWidth - 6) + " //\n"; + header += "// " + + (bareText + padding).slice(0, bannerWidth - 6) + " //\n"; } - buf += spacer + divider + blankLine; + header += spacer + divider + blankLine; + chunks.push(header); // Code, with line numbers // You might prefer your line numbers at the beginning of the @@ -384,26 +435,41 @@ _.extend(File.prototype, { var unitSource = self.linkerUnitTransform(unit.source, options.exports); var lines = unitSource.split('\n'); + // There are probably ways to make a more compact source + // map. For example, for an included unit, the only change we + // make is to append a comment, so we can probably emit one + // mapping for the whole unit. And for a non-included unit, we + // can probably tolerate mapping it inexactly or not at all + // (since it's in a comment.) For the moment, we'll do it by the + // book just to see how it goes. + _.each(lines, function (line) { - if (! unit.include) - line = "// " + line; - if (line.length > width) - buf += line + "\n"; - else - buf += (line + padding).slice(0, width) + " // " + num + "\n"; + var prefix = "", suffix = "\n"; + + if (! unit.include) { + prefix = "// "; + } + + var lengthWithPrefix = line.length + prefix.length; + if (lengthWithPrefix <= width) { + suffix = padding.slice(lengthWithPrefix, width) + " // " + num + "\n"; + } + + chunks.push(prefix); + chunks.push(new sourcemap.SourceNode(num, 0, self._pathForSourceMap(), + line)); + chunks.push(suffix); + num++; }); }); // Footer - buf += divider; + if (! self.bare) + chunks.push(divider + "\n}).call(this);\n"); + chunks.push("\n\n\n\n\n"); - // Epilogue - if (!self.bare) - buf += "\n}).call(this);\n"; - buf += "\n\n\n\n\n"; - - return buf; + return new sourcemap.SourceNode(null, null, null, chunks); }, // If "line" contains nothing but a comment (of either syntax), return the @@ -693,6 +759,8 @@ var link = function (options) { var ret = []; _.each(options.prelinkFiles, function (file) { + // XXX XXX obviously, mucking with boundary ruins the source + // map.. need a new approach here var source = file.source; var parts = source.split(options.boundary); if (parts.length > 2) @@ -702,9 +770,12 @@ var link = function (options) { if (source.length === 0) return; // empty global-imports file -- elide } + ret.push({ source: source, - servePath: file.servePath + servePath: file.servePath, + sourceMap: file.sourceMap, + sources: file.sources }); }); diff --git a/tools/packages.js b/tools/packages.js index ed8d079084..18ea73d828 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -12,6 +12,7 @@ var archinfo = require(path.join(__dirname, 'archinfo.js')); var linker = require(path.join(__dirname, 'linker.js')); var unipackage = require('./unipackage.js'); var fs = require('fs'); +var sourcemap = require('source-map'); // Find all files under `rootPath` that have an extension in // `extensions` (an array of extensions without leading dot), and @@ -180,7 +181,7 @@ var Slice = function (pkg, options) { // source path given in sourceMap (no leading '/') to: // - package: package name, or null for app // - sourcePath: original relative path within 'package' or app - // - path: absolute path on disk to the source file + // - source: full contents of the original source file, as a Buffer // // Set only when isBuilt is true. self.resources = null; @@ -547,7 +548,7 @@ _.extend(Slice.prototype, { var jsResources = _.map(files, function (file) { return { type: "js", - data: new Buffer(file.source, 'utf8'), + data: new Buffer(file.source, 'utf8'), // XXX encoding servePath: file.servePath, staticDirectory: self.staticDirectory sourceMap: file.sourceMap, @@ -1891,7 +1892,18 @@ _.extend(Package.prototype, { if (resource.type === "prelink") { slice.prelinkFiles.push({ source: data.toString('utf8'), - servePath: resource.servePath + servePath: resource.servePath, + // XXX XXX sourceMap and sources should come from separate + // files (and probably be lazily loaded..) + sourceMap: resource.sourceMap ? + sourcemap.SourceMapGenerator.fromSourceMap(new sourcemap.SourceMapConsumer(resource.sourceMap)) : undefined, + sources: resource.sourceMap ? _.map(resource.sources, function (x) { + return { + package: x.package, + sourcePath: x.sourcePath, + source: new Buffer(x.source, 'utf8') + }; + }) : undefined }); } else if (_.contains(["head", "body", "css", "js", "static"], resource.type)) { @@ -2067,7 +2079,17 @@ _.extend(Package.prototype, { file: resourcePath, length: data.length, offset: 0, - servePath: file.servePath || undefined + servePath: file.servePath || undefined, + // XXX XXX these should actually be written to separate files! + // (sourceMap and the actual source files in 'sources' + sourceMap: file.sourceMap ? file.sourceMap.toString() : undefined, + sources: file.sourceMap ? _.map(file.sources, function (x) { + return { + package: x.package, + sourcePath: x.sourcePath, + source: x.source.toString('utf8') + } + }) : undefined }); }); From cab8230a0271367563b39a1630b889d1b0b794e0 Mon Sep 17 00:00:00 2001 From: Geoff Schmidt Date: Mon, 1 Jul 2013 20:35:06 -0700 Subject: [PATCH 04/49] use a private dev bundle version number --- meteor | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meteor b/meteor index be28497200..6be11413e7 100755 --- a/meteor +++ b/meteor @@ -1,6 +1,6 @@ #!/bin/bash -BUNDLE_VERSION=0.3.10 +BUNDLE_VERSION=0.3.9-plus-sourcemap # OS Check. Put here because here is where we download the precompiled # bundles that are arch specific. From 14b337ad5d6f24e7d217b8027ace2f0425360275 Mon Sep 17 00:00:00 2001 From: Geoff Schmidt Date: Mon, 1 Jul 2013 22:24:57 -0700 Subject: [PATCH 05/49] rebase fixups --- tools/bundler.js | 4 ++-- tools/linker.js | 2 +- tools/packages.js | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/bundler.js b/tools/bundler.js index 675b3a4afa..fd45faf31c 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -1097,7 +1097,7 @@ _.extend(JsImage.prototype, { node_modules: item.nodeModulesDirectory ? item.nodeModulesDirectory.preferredBundlePath : undefined, staticDirectory: item.staticDirectory ? - item.staticDirectory.bundlePath : undefined + item.staticDirectory.bundlePath : undefined, sourceMap: item.sourceMap || undefined, // XXX XXX WRONG -- should be a file, not an inline string sources: item.sourceMap ? _.map(item.sources || [], function (x) { return { @@ -1200,7 +1200,7 @@ _.extend(JsImageTarget.prototype, { targetPath: file.targetPath, source: file.contents().toString('utf8'), nodeModulesDirectory: file.nodeModulesDirectory, - staticDirectory: file.staticDirectory + staticDirectory: file.staticDirectory, sourceMap: file.sourceMap, sources: file.sources }); diff --git a/tools/linker.js b/tools/linker.js index 7c4f07b0d8..59fab26842 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -133,7 +133,7 @@ _.extend(Module.prototype, { return ret.concat(_.map(self.files, function (file) { var node = file.getLinkedOutput({ preserveLineNumbers: true, - exports: moduleExports }), + exports: moduleExports }); var results = node.toStringWithSourceMap({ file: file.servePath }); // results has 'code' and 'map' attributes diff --git a/tools/packages.js b/tools/packages.js index 18ea73d828..001af89b35 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -550,7 +550,7 @@ _.extend(Slice.prototype, { type: "js", data: new Buffer(file.source, 'utf8'), // XXX encoding servePath: file.servePath, - staticDirectory: self.staticDirectory + staticDirectory: self.staticDirectory, sourceMap: file.sourceMap, sources: file.sources }; From 32aa96c2323075300e3cbd58c726a09790bade53 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 17:33:14 -0700 Subject: [PATCH 06/49] Add node-source-map-support to dev bundle. --- scripts/generate-dev-bundle.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/scripts/generate-dev-bundle.sh b/scripts/generate-dev-bundle.sh index cca59e5309..68a358257b 100755 --- a/scripts/generate-dev-bundle.sh +++ b/scripts/generate-dev-bundle.sh @@ -111,6 +111,11 @@ npm install shell-quote@0.0.1 npm install byline@2.0.3 npm install source-map@0.1.22 +# Fork of node-source-map-support which allows us to specify our own +# retrieveSourceMap function. +# XXX send a pull request +npm install https://github.com/meteor/node-source-map-support/tarball/3b90243e6b51a2ff8705acd74b62c09df05a6199 + # uglify-js has a bug which drops 'undefined' in arrays: # https://github.com/mishoo/UglifyJS2/pull/97 npm install https://github.com/meteor/UglifyJS2/tarball/aa5abd14d3 From 91142a56365c70de2a4e1b2cca5cea85336ea2a8 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 17:34:22 -0700 Subject: [PATCH 07/49] Upgrade to newer source-map (same as what node-source-map-support uses). --- scripts/generate-dev-bundle.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/generate-dev-bundle.sh b/scripts/generate-dev-bundle.sh index 68a358257b..1a62c2bb67 100755 --- a/scripts/generate-dev-bundle.sh +++ b/scripts/generate-dev-bundle.sh @@ -109,7 +109,7 @@ npm install tar@0.1.14 npm install kexec@0.1.1 npm install shell-quote@0.0.1 npm install byline@2.0.3 -npm install source-map@0.1.22 +npm install source-map@0.1.24 # Fork of node-source-map-support which allows us to specify our own # retrieveSourceMap function. From 30f82aa5bf229cfb5536ae05ae0cf303f6b968e9 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 18:00:23 -0700 Subject: [PATCH 08/49] Rename getLinked[Files/Output] -> getPrelinked$1 --- tools/linker.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/linker.js b/tools/linker.js index 59fab26842..b58b520636 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -114,7 +114,7 @@ _.extend(Module.prototype, { // Output is a list of objects with keys 'source', 'servePath', // 'sourceMap', 'sources' (map from relative path in source map to // 'package', 'sourcePath', 'source') - getLinkedFiles: function () { + getPrelinkedFiles: function () { var self = this; if (! self.files.length && ! self.useGlobalNamespace) @@ -132,8 +132,8 @@ _.extend(Module.prototype, { }]; return ret.concat(_.map(self.files, function (file) { - var node = file.getLinkedOutput({ preserveLineNumbers: true, - exports: moduleExports }); + var node = file.getPrelinkedOutput({ preserveLineNumbers: true, + exports: moduleExports }); var results = node.toStringWithSourceMap({ file: file.servePath }); // results has 'code' and 'map' attributes @@ -171,8 +171,8 @@ _.extend(Module.prototype, { // Emit each file var sources = {}; _.each(self.files, function (file) { - chunks.push(file.getLinkedOutput({ sourceWidth: sourceWidth, - exports: moduleExports })); + chunks.push(file.getPrelinkedOutput({ sourceWidth: sourceWidth, + exports: moduleExports })); chunks.push("\n"); file.addToSourcesSet(sources); }); @@ -382,7 +382,7 @@ _.extend(File.prototype, { // - exports: the module's exports // // Returns a SourceNode. - getLinkedOutput: function (options) { + getPrelinkedOutput: function (options) { var self = this; // The newline after the source closes a '//' comment. @@ -725,7 +725,7 @@ var prelink = function (options) { module.addFile(inputFile); }); - var files = module.getLinkedFiles(); + var files = module.getPrelinkedFiles(); var exports = module.getExports(); return { From b9bf08b45a6dd2598380d1ad61be8c279ac74ace Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 18:05:05 -0700 Subject: [PATCH 09/49] Correctly get the package name in the linker. --- tools/linker.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tools/linker.js b/tools/linker.js index b58b520636..1d378f6873 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -311,10 +311,6 @@ var File = function (inputFile, module) { // package or app.) Used for source maps, error messages.. self.sourcePath = inputFile.sourcePath; - // The source tree to which sourcePath is relative. Either a name of - // a package, or null to mean "the app". - self.package = null; // XXX XXX actually set this - // should line and column be included in errors? self.includePositionInErrors = inputFile.includePositionInErrors; @@ -357,8 +353,8 @@ _.extend(File.prototype, { _pathForSourceMap: function () { var self = this; - if (self.package) - return "package/" + self.package + "/" + self.sourcePath; + if (self.module.name) + return "package/" + self.module.name + "/" + self.sourcePath; else return "app/" + self.sourcePath; }, @@ -368,7 +364,7 @@ _.extend(File.prototype, { addToSourcesSet: function (sources) { var self = this; sources[self._pathForSourceMap()] = { - package: self.package, + package: self.module.name, sourcePath: self.sourcePath, source: new Buffer(self.source, 'utf8') // XXX encoding }; From 09cb871d15780d6764e4eddcad46bc373f67cb68 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 18:24:26 -0700 Subject: [PATCH 10/49] Write slice JSON with source map and sources as separate files. We have not written the corresponding read code yet. --- tools/linker.js | 2 ++ tools/packages.js | 39 ++++++++++++++++++++++++++++----------- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/tools/linker.js b/tools/linker.js index 1d378f6873..fb8ca52372 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -353,6 +353,8 @@ _.extend(File.prototype, { _pathForSourceMap: function () { var self = this; + // XXX why is this "package" and not "packages"? Every other directory is + // called "packages". if (self.module.name) return "package/" + self.module.name + "/" + self.sourcePath; else diff --git a/tools/packages.js b/tools/packages.js index 001af89b35..8c9d272ec3 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -2074,23 +2074,40 @@ _.extend(Package.prototype, { data: data }); - sliceJson.resources.push({ + var resource = { type: 'prelink', file: resourcePath, length: data.length, offset: 0, - servePath: file.servePath || undefined, - // XXX XXX these should actually be written to separate files! - // (sourceMap and the actual source files in 'sources' - sourceMap: file.sourceMap ? file.sourceMap.toString() : undefined, - sources: file.sourceMap ? _.map(file.sources, function (x) { - return { + servePath: file.servePath || undefined + }; + + if (file.sourceMap) { + // Write the source map. + var mapFilename = builder.generateFilename( + path.join(sliceDir, file.servePath + '.map')); + resource.sourceMap = mapFilename; + builder.write(mapFilename, { + data: new Buffer(file.sourceMap.toString(), 'utf8') + }); + + // Now write the sources themselves. + resource.sources = {}; + _.each(file.sources, function (x, pathForSourceMap) { + var savedFilename = builder.generateFilename( + path.join(sliceDir, 'sources', x.sourcePath)); + builder.write(savedFilename, { + data: x.source + }); + resource.sources[pathForSourceMap] = { package: x.package, sourcePath: x.sourcePath, - source: x.source.toString('utf8') - } - }) : undefined - }); + source: savedFilename + }; + }); + } + + sliceJson.resources.push(resource); }); // If slice has included node_modules, copy them in From 36454824ac29e5ae992c2ba9b4d8391f59b81260 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 18:24:37 -0700 Subject: [PATCH 11/49] remove redundant use line --- packages/meteor/package.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/meteor/package.js b/packages/meteor/package.js index 1d7cfb9622..95b38dfe9f 100644 --- a/packages/meteor/package.js +++ b/packages/meteor/package.js @@ -24,7 +24,6 @@ Package.on_use(function (api, where) { // dynamic variables, bindEnvironment // XXX move into a separate package? - api.use('underscore', ['client', 'server']); api.add_files('dynamics_browser.js', 'client'); api.add_files('dynamics_nodejs.js', 'server'); From 366d4439d81240942ad69d611cd08e17cae071cc Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 19:02:30 -0700 Subject: [PATCH 12/49] read fixed unipackage format --- tools/packages.js | 55 ++++++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/tools/packages.js b/tools/packages.js index 8c9d272ec3..993cc145f5 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -62,6 +62,12 @@ var scanForSources = function (rootPath, extensions, ignoreFiles) { }); }; +var rejectBadPath = function (p) { + if (p.match(/\.\./)) + throw new Error("bad path: " + p); +}; + + /////////////////////////////////////////////////////////////////////////////// // Slice /////////////////////////////////////////////////////////////////////////////// @@ -1805,8 +1811,8 @@ _.extend(Package.prototype, { self.testSlices = mainJson.testSlices; _.each(mainJson.plugins, function (pluginMeta) { - if (pluginMeta.path.match(/\.\./)) - throw new Error("bad path in unipackage"); + rejectBadPath(pluginMeta.path); + var plugin = bundler.readJsImage(path.join(dir, pluginMeta.path)); if (! archinfo.matches(archinfo.host(), plugin.arch)) { @@ -1832,8 +1838,7 @@ _.extend(Package.prototype, { _.each(mainJson.slices, function (sliceMeta) { // aggressively sanitize path (don't let it escape to parent // directory) - if (sliceMeta.path.match(/\.\./)) - throw new Error("bad path in unipackage"); + rejectBadPath(sliceMeta.path); var sliceJson = JSON.parse( fs.readFileSync(path.join(dir, sliceMeta.path))); var sliceBasePath = path.dirname(path.join(dir, sliceMeta.path)); @@ -1844,8 +1849,7 @@ _.extend(Package.prototype, { var nodeModulesPath = null; if (sliceJson.node_modules) { - if (sliceJson.node_modules.match(/\.\./)) - throw new Error("bad node_modules path in unipackage"); + rejectBadPath(sliceJson.node_modules); nodeModulesPath = path.join(sliceBasePath, sliceJson.node_modules); } @@ -1875,8 +1879,7 @@ _.extend(Package.prototype, { slice.resources = []; _.each(sliceJson.resources, function (resource) { - if (resource.file.match(/\.\./)) - throw new Error("bad resource file path in unipackage"); + rejectBadPath(resource.file); var fd = fs.openSync(path.join(sliceBasePath, resource.file), "r"); try { @@ -1890,21 +1893,29 @@ _.extend(Package.prototype, { throw new Error("couldn't read entire resource"); if (resource.type === "prelink") { - slice.prelinkFiles.push({ + var prelinkFile = { source: data.toString('utf8'), - servePath: resource.servePath, - // XXX XXX sourceMap and sources should come from separate - // files (and probably be lazily loaded..) - sourceMap: resource.sourceMap ? - sourcemap.SourceMapGenerator.fromSourceMap(new sourcemap.SourceMapConsumer(resource.sourceMap)) : undefined, - sources: resource.sourceMap ? _.map(resource.sources, function (x) { - return { - package: x.package, - sourcePath: x.sourcePath, - source: new Buffer(x.source, 'utf8') - }; - }) : undefined - }); + servePath: resource.servePath + }; + if (resource.sourceMap) { + rejectBadPath(resource.sourceMap); + var rawSourceMap = fs.readFileSync( + path.join(sliceBasePath, resource.sourceMap), 'utf8'); + prelinkFile.sourceMap = sourcemap.SourceMapGenerator.fromSourceMap( + new sourcemap.SourceMapConsumer(rawSourceMap)); + if (resource.sources) { + resource.sources = {}; + _.each(resource.sources, function (x, pathForSourceMap) { + rejectBadPath(x.source); + resource.sources[pathForSourceMap] = { + package: x.package, + sourcePath: x.sourcePath, + source: fs.readFileSync(path.join(sliceBasePath, x.source)) + }; + }); + } + } + slice.prelinkFiles.push(prelinkFile); } else if (_.contains(["head", "body", "css", "js", "static"], resource.type)) { slice.resources.push({ From e61c64e26b4cf948ae5518877932b2359c515b33 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 19:07:30 -0700 Subject: [PATCH 13/49] the path is packages/foo, not package/foo --- tools/library.js | 4 ++-- tools/linker.js | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/library.js b/tools/library.js index c6083d791d..040cbf645e 100644 --- a/tools/library.js +++ b/tools/library.js @@ -7,7 +7,7 @@ var bundler = require('./bundler.js'); var buildmessage = require('./buildmessage.js'); var fs = require('fs'); -// Under the hood, packages in the library (/package/foo), and user +// Under the hood, packages in the library (/packages/foo), and user // applications, are both Packages -- they are just represented // differently on disk. @@ -374,4 +374,4 @@ _.extend(exports, { return out; } -}); \ No newline at end of file +}); diff --git a/tools/linker.js b/tools/linker.js index fb8ca52372..c5b88e4695 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -353,10 +353,8 @@ _.extend(File.prototype, { _pathForSourceMap: function () { var self = this; - // XXX why is this "package" and not "packages"? Every other directory is - // called "packages". if (self.module.name) - return "package/" + self.module.name + "/" + self.sourcePath; + return "packages/" + self.module.name + "/" + self.sourcePath; else return "app/" + self.sourcePath; }, From 0ceb174ae065428b289d6f7288350a7b5d2ed523 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 19:47:17 -0700 Subject: [PATCH 14/49] Write sourcemaps and source files to JSImages. Have not done the reading code yet. --- tools/bundler.js | 67 ++++++++++++++++++++++++++++++----------------- tools/packages.js | 4 +-- 2 files changed, 45 insertions(+), 26 deletions(-) diff --git a/tools/bundler.js b/tools/bundler.js index fd45faf31c..6ee86e34e3 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -188,6 +188,7 @@ var builder = require(path.join(__dirname, 'builder.js')); var unipackage = require(path.join(__dirname, 'unipackage.js')); var Fiber = require('fibers'); var Future = require(path.join('fibers', 'future')); +var sourcemap = require('source-map'); // files to ignore when bundling. node has no globs, so use regexps var ignoreFiles = [ @@ -259,8 +260,9 @@ var File = function (options) { // disk.) self.sourcePath = options.sourcePath; - // If this file was generated, a sourceMap (provided as a string) - // with debugging information. Set with setSourceMap. + // If this file was generated, a sourceMap (provided as a + // sourcemap.SourceMapGenerator) with debugging information. Set with + // setSourceMap. self.sourceMap = null; // If sourceMap is set, this is a map from a relative source file @@ -390,12 +392,12 @@ _.extend(File.prototype, { }, // Set a source map for this File. sourceMap is given as a - // string. See self.sources for the format of sources. + // sourcemap.SourceMapGenerator. See self.sources for the format of sources. setSourceMap: function (sourceMap, sources) { var self = this; - if (typeof sourceMap !== "string") - throw new Error("sourceMap must be given as a string"); + if (!(sourceMap instanceof sourcemap.SourceMapGenerator)) + throw new Error("sourceMap must be given as a SourceMapGenerator"); self.sourceMap = sourceMap; self.sources = sources || {}; } @@ -469,8 +471,7 @@ _.extend(Target.prototype, { test: options.test || [] }); - // Link JavaScript, put resources in load order, and copy them to - // the bundle + // Link JavaScript and set up self.js, etc. self._emitResources(); // Minify, if requested @@ -612,9 +613,8 @@ _.extend(Target.prototype, { } }, - // Sort the slices in dependency order, then, slice by slice, write - // their resources into the bundle (which includes running the - // JavaScript linker.) + // Process all of the sorted slices (which includes running the JavaScript + // linker). _emitResources: function () { var self = this; @@ -673,8 +673,7 @@ _.extend(Target.prototype, { } if (resource.type === "js" && resource.sourceMap) { - f.setSourceMap(resource.sourceMap.toString(), - resource.sources); + f.setSourceMap(resource.sourceMap, resource.sources); } self[resource.type].push(f); @@ -1071,7 +1070,7 @@ _.extend(JsImage.prototype, { write: function (builder) { var self = this; - builder.reserve("program.js"); + builder.reserve("program.json"); // Finalize choice of paths for node_modules directories -- These // paths are no longer just "preferred"; they are the final paths @@ -1091,22 +1090,42 @@ _.extend(JsImage.prototype, { if (! item.targetPath) throw new Error("No targetPath?"); - builder.write(item.targetPath, { data: new Buffer(item.source, 'utf8') }); - load.push({ - path: item.targetPath, + var loadPath = builder.generateFilename(item.targetPath); + builder.write(loadPath, { data: new Buffer(item.source, 'utf8') }); + var loadItem = { + path: loadPath, node_modules: item.nodeModulesDirectory ? item.nodeModulesDirectory.preferredBundlePath : undefined, staticDirectory: item.staticDirectory ? - item.staticDirectory.bundlePath : undefined, - sourceMap: item.sourceMap || undefined, // XXX XXX WRONG -- should be a file, not an inline string - sources: item.sourceMap ? _.map(item.sources || [], function (x) { - return { - source: x.source.toString('utf8'), // XXX XXX WRONG -- should be a file, not an inline string + item.staticDirectory.bundlePath : undefined + }; + + if (item.sourceMap) { + // XXX this code is very similar to saveAsUnipackage. + // Write the source map. + var mapFilename = builder.generateFilename(item.targetPath + '.map'); + loadItem.sourceMap = mapFilename; + builder.write(mapFilename, { + data: new Buffer(item.sourceMap.toString(), 'utf8') + }); + + // Now write the sources themselves. + loadItem.sources = {}; + _.each(item.sources, function (x, pathForSourceMap) { + var savedFilename = builder.generateFilename( + path.join('sources', pathForSourceMap)); + builder.write(savedFilename, { + data: x.source + }); + loadItem.sources[pathForSourceMap] = { package: x.package, - sourcePath: x.sourcePath + sourcePath: x.sourcePath, + source: savedFilename }; - }) : undefined - }); + }); + } + + load.push(loadItem); }); // node_modules resources from the packages. Due to appropriate diff --git a/tools/packages.js b/tools/packages.js index 993cc145f5..b86f42709c 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -1904,10 +1904,10 @@ _.extend(Package.prototype, { prelinkFile.sourceMap = sourcemap.SourceMapGenerator.fromSourceMap( new sourcemap.SourceMapConsumer(rawSourceMap)); if (resource.sources) { - resource.sources = {}; + prelinkFile.sources = {}; _.each(resource.sources, function (x, pathForSourceMap) { rejectBadPath(x.source); - resource.sources[pathForSourceMap] = { + prelinkFile.sources[pathForSourceMap] = { package: x.package, sourcePath: x.sourcePath, source: fs.readFileSync(path.join(sliceBasePath, x.source)) From cb4db20d708eb6db1aa53fa1ba60df82ca435057 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 19:55:40 -0700 Subject: [PATCH 15/49] Fix reading JS Images. --- tools/bundler.js | 39 +++++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/tools/bundler.js b/tools/bundler.js index 6ee86e34e3..ce62d14ca3 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -206,6 +206,11 @@ var inherits = function (child, parent) { child.prototype.constructor = child; }; +var rejectBadPath = function (p) { + if (p.match(/\.\./)) + throw new Error("bad path: " + p); +}; + /////////////////////////////////////////////////////////////////////////////// // NodeModulesDirectory /////////////////////////////////////////////////////////////////////////////// @@ -941,9 +946,9 @@ var JsImage = function () { // - source: JS source code to load, as a string // - nodeModulesDirectory: a NodeModulesDirectory indicating which // directory should be searched by Npm.require() - // - sourceMap: if set, source map for this code, AS A STRING + // - sourceMap: if set, source map for this code, as a SourceMapGenerator // - sources: map from relative path in source map to object with - // keys 'source' (a string), 'package', 'sourcePath' + // keys 'source' (a Buffer), 'package', 'sourcePath' // note: this can't be called `load` at it would shadow `load()` self.jsToLoad = []; @@ -1166,11 +1171,11 @@ JsImage.readFromDisk = function (controlFilePath) { ret.arch = json.arch; _.each(json.load, function (item) { - if (item.path.match(/\.\./)) - throw new Error("bad path in plugin bundle"); + rejectBadPath(item.path); var nmd = undefined; if (item.node_modules) { + rejectBadPath(item.node_modules); var node_modules = path.join(dir, item.node_modules); if (! (node_modules in ret.nodeModulesDirectories)) { ret.nodeModulesDirectories[node_modules] = @@ -1182,16 +1187,34 @@ JsImage.readFromDisk = function (controlFilePath) { nmd = ret.nodeModulesDirectories[node_modules]; } - ret.jsToLoad.push({ + var loadItem = { targetPath: item.path, source: fs.readFileSync(path.join(dir, item.path)), nodeModulesDirectory: nmd, staticDirectory: new StaticDirectory({ sourcePath: item.staticDirectory }) - // XXX XXX set 'sourceMap' (to a string) and 'sources' (keys - // 'package', 'sourcePath', 'source' as a string) - }); + }; + if (item.sourceMap) { + // XXX this is the same code as initFromUnipackage + rejectBadPath(item.sourceMap); + var rawSourceMap = fs.readFileSync( + path.join(dir, item.sourceMap), 'utf8'); + loadItem.sourceMap = sourcemap.SourceMapGenerator.fromSourceMap( + new sourcemap.SourceMapConsumer(rawSourceMap)); + if (item.sources) { + loadItem.sources = {}; + _.each(item.sources, function (x, pathForSourceMap) { + rejectBadPath(x.source); + loadItem.sources[pathForSourceMap] = { + package: x.package, + sourcePath: x.sourcePath, + source: fs.readFileSync(path.join(dir, x.source)) + }; + }); + } + } + ret.jsToLoad.push(loadItem); }); return ret; From d05e83d958d669f0fc16eca37706ad0e647975ac Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 20:32:31 -0700 Subject: [PATCH 16/49] paths in program.json should not start with /. --- tools/builder.js | 5 +++-- tools/bundler.js | 19 +++++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/tools/builder.js b/tools/builder.js index ba7a00b7d1..a462332fc3 100644 --- a/tools/builder.js +++ b/tools/builder.js @@ -378,6 +378,7 @@ _.extend(Builder.prototype, { var methods = ["write", "writeJson", "reserve", "generateFilename", "copyDirectory", "enter"]; var ret = {}; + var relPathWithSep = relPath + path.sep; _.each(methods, function (method) { ret[method] = function (/* arguments */) { @@ -401,10 +402,10 @@ _.extend(Builder.prototype, { // sub-bundle, not the parent bundle if (ret.substr(0, 1) === '/') ret = ret.substr(1); - if (ret.substr(0, relPath.length) !== relPath) + if (ret.substr(0, relPathWithSep.length) !== relPathWithSep) throw new Error("generateFilename returned path outside of " + "sub-bundle?"); - ret = ret.substr(relPath.length); + ret = ret.substr(relPathWithSep.length); } return ret; diff --git a/tools/bundler.js b/tools/bundler.js index ce62d14ca3..825b8f7d32 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -370,10 +370,10 @@ _.extend(File.prototype, { setTargetPathFromRelPath: function (relPath) { var self = this; // XXX hack - if (relPath.match(/^\/packages\//) || relPath.match(/^\/static\//)) + if (relPath.match(/^packages\//) || relPath.match(/^static\//)) self.targetPath = relPath; else - self.targetPath = path.join('/app', relPath); + self.targetPath = path.join('app', relPath); }, setStaticDirectory: function (relPath, staticSourceDirectory) { @@ -383,12 +383,12 @@ _.extend(File.prototype, { // inside private/) go in static/app/. // XXX same hack as above var bundlePath; - if (relPath.match(/^\/packages\//)) { + if (relPath.match(/^packages\//)) { var dir = path.dirname(relPath); var base = path.basename(relPath, ".js"); - bundlePath = path.join('/static', dir, base); + bundlePath = path.join('static', dir, base); } else { - bundlePath = path.join('/static', 'app'); + bundlePath = path.join('static', 'app'); } self.staticDirectory = new StaticDirectory({ sourcePath: staticSourceDirectory, @@ -652,9 +652,12 @@ _.extend(Target.prototype, { } else if (isNative) { var relPath; if (resource.type === "static") - relPath = path.join(path.sep, "static", resource.servePath); - else - relPath = resource.servePath; + relPath = path.join("static", resource.servePath); + else { + if (resource.servePath.charAt(0) !== '/') + throw new Error("bad servePath: " + resource.servePath); + relPath = resource.servePath.slice(1); + } f.setTargetPathFromRelPath(relPath); if (resource.type === "js") f.setStaticDirectory(relPath, resource.staticDirectory); From 7456124102974b5b8a044b4103a9bfecde1d31a9 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 20:53:41 -0700 Subject: [PATCH 17/49] Node stack traces are now rewritten based on (slightly wrong) source maps. Also, assume that the program.json given to boot.js is relative to CWD, not source file. --- tools/bundler.js | 2 +- tools/server/boot.js | 42 ++++++++++++++++++++++++++++++------- tools/tests/test_bundler.js | 2 +- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/tools/bundler.js b/tools/bundler.js index 825b8f7d32..8e0c9e163a 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -1483,7 +1483,7 @@ var writeSiteArchive = function (targets, outputPath, options) { // Affordances for standalone use if (targets.server) { // add program.json as the first argument after "node main.js" to the boot script. - var stub = new Buffer("process.argv.splice(2, 0, 'program.json');\nrequire('./programs/server/boot.js');\n", 'utf8'); + var stub = new Buffer("process.argv.splice(2, 0, 'program.json');\nprocess.chdir(require('path').join(__dirname, 'programs', 'server'));\nrequire('./programs/server/boot.js');\n", 'utf8'); builder.write('main.js', { data: stub }); builder.write('README', { data: new Buffer( diff --git a/tools/server/boot.js b/tools/server/boot.js index c4dce95b1a..587ab6e3f4 100644 --- a/tools/server/boot.js +++ b/tools/server/boot.js @@ -3,6 +3,7 @@ var fs = require("fs"); var path = require("path"); var Future = require(path.join("fibers", "future")); var _ = require('underscore'); +var sourcemap_support = require('source-map-support'); // This code is duplicated in tools/server/server.js. var MIN_NODE_VERSION = 'v0.8.24'; @@ -13,15 +14,16 @@ if (require('semver').lt(process.version, MIN_NODE_VERSION)) { } // read our control files -var serverJson = - JSON.parse(fs.readFileSync(path.join(__dirname, process.argv[2]), 'utf8')); +var serverJsonPath = path.resolve(process.argv[2]); +var serverDir = path.dirname(serverJsonPath); +var serverJson = JSON.parse(fs.readFileSync(serverJsonPath, 'utf8')); var configJson = - JSON.parse(fs.readFileSync(path.join(__dirname, 'config.json'), 'utf8')); + JSON.parse(fs.readFileSync(path.resolve(serverDir, 'config.json'), 'utf8')); // Set up environment __meteor_bootstrap__ = { startup_hooks: [], - serverDir: __dirname, + serverDir: serverDir, configJson: configJson }; __meteor_runtime_config__ = { meteorRelease: configJson.release }; @@ -34,10 +36,36 @@ __meteor_runtime_config__ = { meteorRelease: configJson.release }; if (!process.env.NODE_ENV) process.env.NODE_ENV = 'production'; +// Map from load path to its source map. +var sourceMapsAsStrings = {}; + +// Read all the source maps into memory once. +_.each(serverJson.load, function (fileInfo) { + if (fileInfo.sourceMap) { + sourceMapsAsStrings[fileInfo.path] = fs.readFileSync( + path.resolve(serverDir, fileInfo.sourceMap), 'utf8'); + } +}); + +var retrieveSourceMap = function (pathForSourceMap) { + if (_.has(sourceMapsAsStrings, pathForSourceMap)) + return {data: sourceMapsAsStrings[pathForSourceMap]}; + return null; +}; + +sourcemap_support.install({ + // Use the source maps specified in program.json instead of parsing source + // code for them. + retrieveSourceMap: retrieveSourceMap, + // For now, don't fix the source line in uncaught exceptions, because we + // haven't fixed handleUncaughtExceptions in source-map-support to properly + // locate the source files. + handleUncaughtExceptions: false +}); Fiber(function () { _.each(serverJson.load, function (fileInfo) { - var code = fs.readFileSync(path.join(__dirname, fileInfo.path)); + var code = fs.readFileSync(path.resolve(serverDir, fileInfo.path)); var Npm = { require: function (name) { @@ -46,7 +74,7 @@ Fiber(function () { } var nodeModuleDir = - path.join(__dirname, fileInfo.node_modules, name); + path.resolve(serverDir, fileInfo.node_modules, name); if (fs.existsSync(nodeModuleDir)) { return require(nodeModuleDir); @@ -67,7 +95,7 @@ Fiber(function () { } } }; - var staticDirectory = path.join(__dirname, fileInfo.staticDirectory); + var staticDirectory = path.resolve(serverDir, fileInfo.staticDirectory); var getAsset = function (assetPath, encoding, callback) { var fut; if (! callback) { diff --git a/tools/tests/test_bundler.js b/tools/tests/test_bundler.js index 434c028fad..0e9b24e3c9 100644 --- a/tools/tests/test_bundler.js +++ b/tools/tests/test_bundler.js @@ -12,7 +12,7 @@ /*global*/ Fiber = require('fibers'); /*global*/ Future = require('fibers/future'); -/*global*/ mainJSContents = "process.argv.splice(2, 0, 'program.json');\nrequire('./programs/server/boot.js');\n"; +/*global*/ mainJSContents = "process.argv.splice(2, 0, 'program.json');\nprocess.chdir(require('path').join(__dirname, 'programs', 'server'));\nrequire('./programs/server/boot.js');\n"; var tmpBaseDir = files.mkdtemp('test_bundler'); var tmpCounter = 1; From b3e752c86c2d2a8a6cf4c5f9d504d8db8c482be2 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 22:30:02 -0700 Subject: [PATCH 18/49] Source maps now are correct. The header and footer of the linked file now is generated entirely at link time. There is no more "boundary" __imports__asfdadsads blob. Also, fix an inexplicable typo in js_analyze. --- packages/js-analyze/js_analyze.js | 2 +- tools/linker.js | 203 ++++++++++++++---------------- tools/packages.js | 33 ++--- 3 files changed, 114 insertions(+), 124 deletions(-) diff --git a/packages/js-analyze/js_analyze.js b/packages/js-analyze/js_analyze.js index c1d7ba6a02..3a142fbfb5 100644 --- a/packages/js-analyze/js_analyze.js +++ b/packages/js-analyze/js_analyze.js @@ -148,7 +148,7 @@ JSAnalyze.findAssignedGlobals = function (source) { // causes escope to not bother to resolve references in the eval's scope. // This is because an eval can pull references inward: // - / function outer() { + // function outer() { // var i = 42; // function inner() { // eval('var i = 0'); diff --git a/tools/linker.js b/tools/linker.js index c5b88e4695..bdab736564 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -10,21 +10,6 @@ var packageDot = function (name) { return "Package['" + name + "']"; }; -var generateBoundary = function () { - // In a perfect world we would call Packages.random.Random.id(). - // But we can't do that this is part of the code that is used to - // compile and load packages. So let it slide for now and provide a - // version based on (the completely non-cryptographic) Math.random, - // which is good enough for this particular application. - var alphabet = "23456789ABCDEFGHJKLMNPQRSTWXYZabcdefghijkmnopqrstuvwxyz"; - var digits = []; - for (var i = 0; i < 17; i++) { - var index = Math.floor(Math.random() * alphabet.length); - digits[i] = alphabet.substr(index, 1); - } - return "__imports_" + digits.join("") + "__"; -}; - /////////////////////////////////////////////////////////////////////////////// // Module /////////////////////////////////////////////////////////////////////////////// @@ -41,9 +26,6 @@ var Module = function (options) { // files in the module. array of File self.files = []; - // boundary to use to mark where import should go in final phase - self.boundary = generateBoundary(); - // options self.forceExport = options.forceExport || []; self.useGlobalNamespace = options.useGlobalNamespace; @@ -88,13 +70,13 @@ _.extend(Module.prototype, { // and see what your globals are. Probably this means we need to // move the emission of the Package-scope Variables section (but not // the actual static analysis) to the final phase. - computeModuleScopedVars: function () { + computeModuleScopeVars: function () { var self = this; if (!self.jsAnalyze) { // We don't have access to static analysis, probably because we *are* the // js-analyze package. Let's do a stupid heuristic: any exports that have - // no dots are module scoped vars. (This works for + // no dots are module scope vars. (This works for // js-analyze.JSAnalyze...) return _.filter(self.getExports(), function (e) { return e.indexOf('.') === -1; @@ -108,7 +90,7 @@ _.extend(Module.prototype, { }); globalReferences = _.uniq(globalReferences); - return globalReferences; + return _.isEmpty(globalReferences) ? undefined : globalReferences; }, // Output is a list of objects with keys 'source', 'servePath', @@ -117,7 +99,7 @@ _.extend(Module.prototype, { getPrelinkedFiles: function () { var self = this; - if (! self.files.length && ! self.useGlobalNamespace) + if (! self.files.length) return []; var moduleExports = self.getExports(); @@ -126,12 +108,7 @@ _.extend(Module.prototype, { // then our job is much simpler. And we can get away with // preserving the line numbers. if (self.useGlobalNamespace) { - var ret = [{ - source: self.boundary, - servePath: self.importStubServePath - }]; - - return ret.concat(_.map(self.files, function (file) { + return _.map(self.files, function (file) { var node = file.getPrelinkedOutput({ preserveLineNumbers: true, exports: moduleExports }); var results = node.toStringWithSourceMap({ @@ -147,7 +124,7 @@ _.extend(Module.prototype, { sourceMap: results.map, sources: sources }; - })); + }); } // Otherwise.. @@ -156,31 +133,19 @@ _.extend(Module.prototype, { // comments that will be emitted when we skip a unit. var sourceWidth = _.max([68, self.maxLineLength(120 - 2)]) + 3; - // Figure out which variables are module scope - var moduleScopedVars = self.computeModuleScopedVars(); - // Prologue var chunks = []; - chunks.push("(function () {\n\n" + self.boundary); - - if (moduleScopedVars.length) { - chunks.push("/* Package-scope variables */\n"); - chunks.push("var " + moduleScopedVars.join(', ') + ";\n\n"); - } // Emit each file var sources = {}; _.each(self.files, function (file) { + if (!_.isEmpty(chunks)) + chunks.push("\n\n\n\n\n\n"); chunks.push(file.getPrelinkedOutput({ sourceWidth: sourceWidth, exports: moduleExports })); - chunks.push("\n"); file.addToSourcesSet(sources); }); - // Epilogue - chunks.push(self.getExportCode()); - chunks.push("\n})();"); - var node = new sourcemap.SourceNode(null, null, null, chunks); var results = node.toStringWithSourceMap({ file: self.combinedServePath @@ -209,45 +174,7 @@ _.extend(Module.prototype, { }); return _.union(_.keys(exports), self.forceExport); - }, - - // Return code that saves our exports to Package.packagename.foo.bar - getExportCode: function () { - var self = this; - if (! self.name) - return ""; - // If we're a no-exports module, then we have no export code (not even - // creating Package.foo). - if (self.noExports) - return ""; - if (self.useGlobalNamespace) - // Haven't thought about this case. When would this happen? - throw new Error("Not implemented: exports from global namespace"); - - var buf = "/* Exports */\n"; - buf += "if (typeof Package === 'undefined') Package = {};\n"; - buf += packageDot(self.name) + " = "; - - var exports = self.getExports(); - // Even if there are no exports, we need to define Package.foo, because the - // existence of Package.foo is how another package (eg, one that weakly - // depends on foo) can tell if foo is loaded. - if (exports.length === 0) - return buf + "{};\n"; - - // Given exports like Foo, Bar.Baz, Bar.Quux.A, and Bar.Quux.B, - // construct an expression like - // {Foo: Foo, Bar: {Baz: Bar.Baz, Quux: {A: Bar.Quux.A, B: Bar.Quux.B}}} - var scratch = {}; - _.each(self.getExports(), function (symbol) { - scratch[symbol] = symbol; - }); - var exportTree = buildSymbolTree(scratch); - buf += writeSymbolTree(exportTree, 0); - buf += ";\n"; - return buf; } - }); // Given 'symbolMap' like {Foo: 's1', 'Bar.Baz': 's2', 'Bar.Quux.A': 's3', 'Bar.Quux.B': 's4'} @@ -463,7 +390,6 @@ _.extend(File.prototype, { // Footer if (! self.bare) chunks.push(divider + "\n}).call(this);\n"); - chunks.push("\n\n\n\n\n"); return new sourcemap.SourceNode(null, null, null, chunks); }, @@ -700,7 +626,6 @@ _.extend(Unit.prototype, { // sourceMap (a SourceMapGenerator) and sources (map to keys 'package', // 'sourcePath', 'path') similar to self.resources in a Slice (XXX) // - exports: the exports, as a list of string ('Foo', 'Thing.Stuff', etc) -// - boundary: an opaque value that must be passed along with 'files' to link() var prelink = function (options) { if (options.noExports && options.forceExport && ! _.isEmpty(options.forceExport)) { @@ -723,11 +648,12 @@ var prelink = function (options) { var files = module.getPrelinkedFiles(); var exports = module.getExports(); + var packageScopeVariables = module.computeModuleScopeVars(); return { files: files, exports: exports, - boundary: module.boundary + packageScopeVariables: packageScopeVariables }; }; @@ -744,40 +670,70 @@ var prelink = function (options) { // // prelinkFiles: the 'files' output from prelink() // -// boundary: the 'boundary' output from prelink() -// // Output is an array of final output files in the same format as the // 'inputFiles' argument to prelink(). var link = function (options) { - var importCode = options.useGlobalNamespace ? - getImportCode(options.imports, "/* Imports for global scope */\n\n", true) : - getImportCode(options.imports, "/* Imports */\n"); + if (options.useGlobalNamespace) { + return [{ + source: getImportCode(options.imports, + "/* Imports for global scope */\n\n", true), + servePath: options.importStubServePath + }].concat(options.prelinkFiles); + } + + var header = getHeader({ + imports: options.imports, + packageScopeVariables: options.packageScopeVariables}); + var footer = getFooter({ + exports: options.exports, + name: options.name + }); var ret = []; _.each(options.prelinkFiles, function (file) { - // XXX XXX obviously, mucking with boundary ruins the source - // map.. need a new approach here - var source = file.source; - var parts = source.split(options.boundary); - if (parts.length > 2) - throw new Error("Boundary appears more than once?"); - if (parts.length === 2) { - source = parts[0] + importCode + parts[1]; - if (source.length === 0) - return; // empty global-imports file -- elide + if (file.sourceMap) { + // XXX we read this as a string (in initFromUnipackage), then converted to + // Consumer and then to Generator. It's wasteful to then convert back to + // string and to consumer. + var node = new sourcemap.SourceNode(null, null, null, [ + header, + sourcemap.SourceNode.fromStringWithSourceMap( + file.source, + new sourcemap.SourceMapConsumer(file.sourceMap.toString())), + footer + ]); + var results = node.toStringWithSourceMap({ + file: file.servePath + }); + ret.push({ + source: results.code, + servePath: file.servePath, + sourceMap: results.map, + sources: file.sources + }); + } else { + ret.push({ + source: header + file.source + footer, + servePath: file.servePath + }); } - - ret.push({ - source: source, - servePath: file.servePath, - sourceMap: file.sourceMap, - sources: file.sources - }); }); return ret; }; +var getHeader = function (options) { + var chunks = []; + chunks.push("(function () {\n\n" ); + chunks.push(getImportCode(options.imports, "/* Imports */\n")); + if (options.packageScopeVariables + && !_.isEmpty(options.packageScopeVariables)) { + chunks.push("/* Package-scope variables */\n"); + chunks.push("var " + options.packageScopeVariables.join(', ') + ";\n\n"); + } + return chunks.join(''); +}; + var getImportCode = function (imports, header, omitvar) { var self = this; @@ -788,10 +744,10 @@ var getImportCode = function (imports, header, omitvar) { _.each(imports, function (name, symbol) { scratch[symbol] = packageDot(name) + "." + symbol; }); - var imports = buildSymbolTree(scratch); + var tree = buildSymbolTree(scratch); var buf = header; - _.each(imports, function (node, key) { + _.each(tree, function (node, key) { buf += (omitvar ? "" : "var " ) + key + " = " + writeSymbolTree(node) + ";\n"; }); @@ -801,6 +757,37 @@ var getImportCode = function (imports, header, omitvar) { return buf; }; +var getFooter = function (options) { + var chunks = []; + + if (options.name && options.exports && !_.isEmpty(options.exports)) { + chunks.push("/* Exports */\n"); + chunks.push("if (typeof Package === 'undefined') Package = {};\n"); + chunks.push(packageDot(options.name), " = "); + + // Even if there are no exports, we need to define Package.foo, because the + // existence of Package.foo is how another package (eg, one that weakly + // depends on foo) can tell if foo is loaded. + if (_.isEmpty(options.exports)) { + chunks.push("{};\n"); + } else { + // Given exports like Foo, Bar.Baz, Bar.Quux.A, and Bar.Quux.B, + // construct an expression like + // {Foo: Foo, Bar: {Baz: Bar.Baz, Quux: {A: Bar.Quux.A, B: Bar.Quux.B}}} + var scratch = {}; + _.each(options.exports, function (symbol) { + scratch[symbol] = symbol; + }); + var exportTree = buildSymbolTree(scratch); + chunks.push(writeSymbolTree(exportTree, 0)); + chunks.push(";\n"); + } + } + + chunks.push("\n})();\n"); + return chunks.join(''); +}; + var linker = module.exports = { prelink: prelink, link: link diff --git a/tools/packages.js b/tools/packages.js index b86f42709c..4a1845cebb 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -154,14 +154,15 @@ var Slice = function (pkg, options) { // Are we allowed to have exports? (eg, test slices don't export.) self.noExports = !!options.noExports; - // Prelink output. 'boundary' is a magic cookie used for inserting - // imports. 'prelinkFiles' is the partially linked JavaScript code - // (an array of objects with keys 'source' and 'servePath', both - // strings -- see prelink() in linker.js) Both of these are inputs - // into the final link phase, which inserts the final JavaScript - // resources into 'resources'. Set only when isBuilt is true. - self.boundary = null; + // Prelink output. 'prelinkFiles' is the partially linked JavaScript code (an + // array of objects with keys 'source' and 'servePath', both strings -- see + // prelink() in linker.js) 'packageScopeVariables' are are variables that are + // syntactically globals in our input files and which we capture with a + // package-scope closure. Both of these are inputs into the final link phase, + // which inserts the final JavaScript resources into 'resources'. Set only + // when isBuilt is true. self.prelinkFiles = null; + self.packageScopeVariables = null; // All of the data provided for eventual inclusion in the bundle, // other than JavaScript that still needs to be fed through the @@ -207,7 +208,7 @@ _.extend(Slice.prototype, { // through the appropriate handlers and run the prelink phase on any // resulting JavaScript. Also add all provided source files to the // package dependencies. Sets fields such as dependencies, exports, - // boundary, prelinkFiles, and resources. + // prelinkFiles, packageScopeVariables, and resources. build: function () { var self = this; var isApp = ! self.pkg.name; @@ -466,8 +467,6 @@ _.extend(Slice.prototype, { combinedServePath: isApp ? null : "/packages/" + self.pkg.name + (self.sliceName === "main" ? "" : ("." + self.sliceName)) + ".js", - // XXX report an error if there is a package called global-imports - importStubServePath: '/packages/global-imports.js', name: self.pkg.name || null, forceExport: self.forceExport, noExports: self.noExports, @@ -491,8 +490,8 @@ _.extend(Slice.prototype, { }); self.prelinkFiles = results.files; - self.boundary = results.boundary; self.exports = results.exports; + self.packageScopeVariables = results.packageScopeVariables; self.resources = resources; self.isBuilt = true; }, @@ -546,8 +545,12 @@ _.extend(Slice.prototype, { var files = linker.link({ imports: imports, useGlobalNamespace: isApp, + // XXX report an error if there is a package called global-imports + importStubServePath: isApp && '/packages/global-imports.js', prelinkFiles: self.prelinkFiles, - boundary: self.boundary + exports: self.exports, + packageScopeVariables: self.packageScopeVariables, + name: self.pkg.name || null }); // Add each output as a resource @@ -1874,7 +1877,7 @@ _.extend(Package.prototype, { slice.isBuilt = true; slice.exports = sliceJson.exports || []; - slice.boundary = sliceJson.boundary; + slice.packageScopeVariables = sliceJson.packageScopeVariables || []; slice.prelinkFiles = []; slice.resources = []; @@ -1968,7 +1971,7 @@ _.extend(Package.prototype, { var buildInfoJson = { dependencies: { files: {}, directories: {} }, - source: options.buildOfPath || undefined, + source: options.buildOfPath || undefined }; builder.reserve("unipackage.json"); @@ -2011,6 +2014,7 @@ _.extend(Package.prototype, { var sliceJson = { format: "unipackage-slice-pre1", exports: slice.exports, + packageScopeVariables: slice.packageScopeVariables, uses: _.map(slice.uses, function (u) { var specParts = u.spec.split('.'); if (specParts.length > 2) @@ -2024,7 +2028,6 @@ _.extend(Package.prototype, { }), node_modules: slice.nodeModulesPath ? 'npm/node_modules' : undefined, resources: [], - boundary: slice.boundary, staticDirectory: path.join(sliceDir, self.serveRoot) }; From f0673cef7e6eab5d192c22ff9958a26fe3179380 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 22:31:01 -0700 Subject: [PATCH 19/49] don't generate empty global import files --- tools/linker.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tools/linker.js b/tools/linker.js index bdab736564..e3d2b631ad 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -674,11 +674,15 @@ var prelink = function (options) { // 'inputFiles' argument to prelink(). var link = function (options) { if (options.useGlobalNamespace) { - return [{ - source: getImportCode(options.imports, - "/* Imports for global scope */\n\n", true), - servePath: options.importStubServePath - }].concat(options.prelinkFiles); + var ret = []; + if (!_.isEmpty(options.imports)) { + ret.push({ + source: getImportCode(options.imports, + "/* Imports for global scope */\n\n", true), + servePath: options.importStubServePath + }); + } + return ret.concat(options.prelinkFiles); } var header = getHeader({ From 34d563a7edc8d483835565b29ff817d57ed7cee6 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 22:49:56 -0700 Subject: [PATCH 20/49] put js image format at top of file --- tools/bundler.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/bundler.js b/tools/bundler.js index 8e0c9e163a..da0fd0fb57 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -1151,9 +1151,9 @@ _.extend(JsImage.prototype, { // Control file builder.writeJson('program.json', { - load: load, format: "javascript-image-pre1", - arch: self.arch + arch: self.arch, + load: load }); return "program.json"; } From 5b8e1c17f3eeaedc95569a2bdf9db41697501b1a Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Jul 2013 23:28:15 -0700 Subject: [PATCH 21/49] Use manifest, not directory serving, to serve static files. --- .../webapp/.npm/package/npm-shrinkwrap.json | 17 ++++ packages/webapp/package.js | 1 + packages/webapp/webapp_server.js | 81 ++++++++++++++----- tools/bundler.js | 24 +----- 4 files changed, 82 insertions(+), 41 deletions(-) diff --git a/packages/webapp/.npm/package/npm-shrinkwrap.json b/packages/webapp/.npm/package/npm-shrinkwrap.json index 8b3977779b..887dcb5e04 100644 --- a/packages/webapp/.npm/package/npm-shrinkwrap.json +++ b/packages/webapp/.npm/package/npm-shrinkwrap.json @@ -43,6 +43,23 @@ } } }, + "send": { + "version": "0.1.0", + "dependencies": { + "debug": { + "version": "0.7.2" + }, + "mime": { + "version": "1.2.6" + }, + "fresh": { + "version": "0.1.0" + }, + "range-parser": { + "version": "0.0.4" + } + } + }, "useragent": { "version": "2.0.1" } diff --git a/packages/webapp/package.js b/packages/webapp/package.js index e22134dfc5..351762533a 100644 --- a/packages/webapp/package.js +++ b/packages/webapp/package.js @@ -4,6 +4,7 @@ Package.describe({ }); Npm.depends({connect: "2.7.10", + send: "0.1.0", useragent: "2.0.1"}); Package.on_use(function (api) { diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js index 0b49d60e60..90094d1261 100644 --- a/packages/webapp/webapp_server.js +++ b/packages/webapp/webapp_server.js @@ -9,6 +9,7 @@ var url = Npm.require("url"); var connect = Npm.require('connect'); var optimist = Npm.require('optimist'); var useragent = Npm.require('useragent'); +var send = Npm.require('send'); // @export WebApp WebApp = {}; @@ -215,26 +216,68 @@ var runWebAppServer = function () { // Auto-compress any json, javascript, or text. app.use(connect.compress()); - if (clientJson.staticCacheable) { - // cacheable files are files that should never change. Typically - // named by their hash (eg meteor bundled js and css files). - // cache them ~forever (1yr) - app.use(connect.static(path.join(clientDir, clientJson.staticCacheable), - {maxAge: 1000 * 60 * 60 * 24 * 365})); - } + var staticFiles = {}; + _.each(clientJson.manifest, function (item) { + if (item.url && item.where === "client") + staticFiles[url.parse(item.url).pathname] = item; + }); - // cache non-cacheable file anyway. This isn't really correct, as - // users can change the files and changes won't propogate - // immediately. However, if we don't cache them, browsers will - // 'flicker' when rerendering images. Eventually we will probably want - // to rewrite URLs of static assets to include a query parameter to - // bust caches. That way we can both get good caching behavior and - // allow users to change assets without delay. - // https://github.com/meteor/meteor/issues/773 - if (clientJson.static) { - app.use(connect.static(path.join(clientDir, clientJson.static), - {maxAge: 1000 * 60 * 60 * 24})); - } + // Serve static files from the manifest. + // This is inspired by the 'static' middleware. + app.use(function (req, res, next) { + if ('GET' != req.method && 'HEAD' != req.method) { + next(); + return; + } + var path = connect.utils.parseUrl(req).pathname; + + try { + path = decodeURIComponent(path); + } catch (e) { + next(); + return; + } + if (!_.has(staticFiles, path)) { + next(); + return; + } + + // We don't need to call pause because, unlike 'static', once we call into + // 'send' and yield to the event loop, we never call another handler with + // 'next'. + + var info = staticFiles[path]; + + // Cacheable files are files that should never change. Typically + // named by their hash (eg meteor bundled js and css files). + // We cache them ~forever (1yr). + // + // We cache non-cacheable files anyway. This isn't really correct, as users + // can change the files and changes won't propagate immediately. However, if + // we don't cache them, browsers will 'flicker' when rerendering + // images. Eventually we will probably want to rewrite URLs of static assets + // to include a query parameter to bust caches. That way we can both get + // good caching behavior and allow users to change assets without delay. + // https://github.com/meteor/meteor/issues/773 + var maxAge = info.cacheable + ? 1000 * 60 * 60 * 24 * 365 + : 1000 * 60 * 60 * 24; + + send(req, path.join(clientDir, info.path)) + .maxage(maxAge) + .hidden(true) // if we specified a dotfile in the manifest, serve it + .on('error', function (err) { + Log.error("Error serving static file " + err); + res.writeHead(500); + res.end(); + }) + .on('directory', function () { + Log.error("Unexpected directory " + info.path); + res.writeHead(500); + res.end(); + }) + .pipe(res); + }); // Packages and apps can add handlers to this via WebApp.connectHandlers. // They are inserted before our default handler. diff --git a/tools/bundler.js b/tools/bundler.js index da0fd0fb57..c7d5fcf439 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -76,20 +76,9 @@ // trigger HTML5 appcache reloads at the right time (if the // 'appcache' package is being used.) // -// - static: a path, relative to program.json, to a directory. If the -// server is too dumb to read 'manifest', it can just serve all of -// the files in this directory (with a relatively short cache -// expiry time.) -// XXX do not use this. It will go away soon. -// -// - static_cacheable: just like 'static' but resources that can be -// cached aggressively (cacheable: true in the manifest) -// XXX do not use this. It will go away soon. -// // Convention: // -// page is 'app.html', static is 'static', and staticCacheable is -// 'static_cacheable'. +// page is 'app.html'. // // // == Format of a program when arch is "native.*" == @@ -914,17 +903,8 @@ _.extend(ClientTarget.prototype, { // Control file builder.writeJson('program.json', { format: "browser-program-pre1", - manifest: manifest, page: 'app.html', - - // XXX the following are for use by 'legacy' (read: current) - // server.js implementations which aren't smart enough to read - // the manifest and instead want all of the resources in a - // directory together so they can just point gzippo at it. we - // should remove this and make the server work from the - // manifest. - static: 'static', - staticCacheable: 'static_cacheable' + manifest: manifest }); return "program.json"; } From f786fd6fa50c29b80b78b2ab0258f709fa9aa033 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 9 Jul 2013 10:42:25 -0700 Subject: [PATCH 22/49] Write (but don't use) sourceMaps (but not sources) for client programs. Add builder.writeToGeneratedFilename helper and use it a lot. --- tools/builder.js | 26 ++++++++++++-- tools/bundler.js | 92 ++++++++++++++++++++++++++++++----------------- tools/packages.js | 39 +++++++------------- 3 files changed, 96 insertions(+), 61 deletions(-) diff --git a/tools/builder.js b/tools/builder.js index a462332fc3..d8febca434 100644 --- a/tools/builder.js +++ b/tools/builder.js @@ -272,6 +272,19 @@ _.extend(Builder.prototype, { return relPath; }, + // Convenience wrapper around generateFilename and write. + // + // (Note that in the object returned by builder.enter, this method + // is patched through directly rather than rewriting its inputs and + // outputs. This is only valid because it does nothing with its inputs + // and outputs other than send pass them to other methods.) + writeToGeneratedFilename: function (relPath, writeOptions) { + var self = this; + var generated = self.generateFilename(relPath); + self.write(generated, writeOptions); + return generated; + }, + // Recursively copy a directory and all of its contents into the // bundle. But if the symlink option was passed to the Builder // constructor, then make a symlink instead, if possible. @@ -377,11 +390,11 @@ _.extend(Builder.prototype, { var self = this; var methods = ["write", "writeJson", "reserve", "generateFilename", "copyDirectory", "enter"]; - var ret = {}; + var subBuilder = {}; var relPathWithSep = relPath + path.sep; _.each(methods, function (method) { - ret[method] = function (/* arguments */) { + subBuilder[method] = function (/* arguments */) { var args = _.toArray(arguments); if (method !== "copyDirectory") { @@ -412,7 +425,14 @@ _.extend(Builder.prototype, { }; }); - return ret; + // Methods that don't have to fix up arguments or return values, because + // they are implemented purely in terms of other methods which do. + var passThroughMethods = ["writeToGeneratedFilename"]; + _.each(passThroughMethods, function (method) { + subBuilder[method] = self[method]; + }); + + return subBuilder; }, // Move the completed bundle into its final location (outputPath) diff --git a/tools/bundler.js b/tools/bundler.js index c7d5fcf439..1e862c7fa9 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -70,6 +70,8 @@ // parameter when used // - size: size of file in bytes // - hash: sha1 hash of the file contents +// - sourceMap: optional path to source map file (relative to program.json) +// - sources: same as in native format (see below) // Additionally there will be an entry with where equal to // "internal", path equal to page (above), and hash equal to the // sha1 of page (before replacements.) Currently this is used to @@ -200,6 +202,13 @@ var rejectBadPath = function (p) { throw new Error("bad path: " + p); }; +var stripLeadingSlash = function (p) { + if (p.charAt(0) !== '/') + throw new Error("bad path: " + p); + return p.slice(1); +}; + + /////////////////////////////////////////////////////////////////////////////// // NodeModulesDirectory /////////////////////////////////////////////////////////////////////////////// @@ -643,9 +652,7 @@ _.extend(Target.prototype, { if (resource.type === "static") relPath = path.join("static", resource.servePath); else { - if (resource.servePath.charAt(0) !== '/') - throw new Error("bad servePath: " + resource.servePath); - relPath = resource.servePath.slice(1); + relPath = stripLeadingSlash(resource.servePath); } f.setTargetPathFromRelPath(relPath); if (resource.type === "js") @@ -868,26 +875,50 @@ _.extend(ClientTarget.prototype, { // the target write: function (builder) { var self = this; - var manifest = []; builder.reserve("program.json"); + builder.reserve("app.html"); - // Resources served via HTTP - _.each(["js", "css", "static"], function (type) { - _.each(self[type], function (file) { - - writeFile(file, builder); - - manifest.push({ - path: file.targetPath, - where: "client", - type: type, - cacheable: file.cacheable, - url: file.url, - size: file.size(), - hash: file.hash() + // Helper to iterate over all resources that we serve over HTTP. + var eachResource = function (f) { + _.each(["js", "css", "static"], function (type) { + _.each(self[type], function (file) { + f(file, type); }); }); + }; + + // Reserve all file names from the manifest, so that interleaved + // generateFilename calls don't overlap with them. + eachResource(function (file, type) { + builder.reserve(file.targetPath); + }); + + // Build up a manifest of all resources served via HTTP. + var manifest = []; + eachResource(function (file, type) { + writeFile(file, builder); + + var manifestItem = { + path: file.targetPath, + where: "client", + type: type, + cacheable: file.cacheable, + url: file.url, + size: file.size(), + hash: file.hash() + }; + + if (file.sourceMap) { + manifestItem.sourceMap = builder.writeToGeneratedFilename( + stripLeadingSlash(file.targetPath + '.map'), { + data: new Buffer(file.sourceMap.toString(), 'utf8') + }); + + // XXX write sources too + } + + manifest.push(manifestItem); }); // HTML boilerplate (the HTML served to make the client load the @@ -1078,8 +1109,9 @@ _.extend(JsImage.prototype, { if (! item.targetPath) throw new Error("No targetPath?"); - var loadPath = builder.generateFilename(item.targetPath); - builder.write(loadPath, { data: new Buffer(item.source, 'utf8') }); + var loadPath = builder.writeToGeneratedFilename( + item.targetPath, + { data: new Buffer(item.source, 'utf8') }); var loadItem = { path: loadPath, node_modules: item.nodeModulesDirectory ? @@ -1089,26 +1121,22 @@ _.extend(JsImage.prototype, { }; if (item.sourceMap) { - // XXX this code is very similar to saveAsUnipackage. // Write the source map. - var mapFilename = builder.generateFilename(item.targetPath + '.map'); - loadItem.sourceMap = mapFilename; - builder.write(mapFilename, { - data: new Buffer(item.sourceMap.toString(), 'utf8') - }); + // XXX this code is very similar to saveAsUnipackage. + loadItem.sourceMap = builder.writeToGeneratedFilename( + item.targetPath + '.map', + { data: new Buffer(item.sourceMap.toString(), 'utf8') } + ); // Now write the sources themselves. loadItem.sources = {}; _.each(item.sources, function (x, pathForSourceMap) { - var savedFilename = builder.generateFilename( - path.join('sources', pathForSourceMap)); - builder.write(savedFilename, { - data: x.source - }); loadItem.sources[pathForSourceMap] = { package: x.package, sourcePath: x.sourcePath, - source: savedFilename + source: builder.writeToGeneratedFilename( + path.join('sources', pathForSourceMap), + { data: x.source }) }; }); } diff --git a/tools/packages.js b/tools/packages.js index 4a1845cebb..eb8d3cd1c4 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -2065,13 +2065,11 @@ _.extend(Package.prototype, { if (_.contains(["head", "body"], resource.type)) return; // already did this one - var resourcePath = builder.generateFilename( - path.join(sliceDir, resource.servePath)); - - builder.write(resourcePath, { data: resource.data }); sliceJson.resources.push({ type: resource.type, - file: resourcePath, + file: builder.writeToGeneratedFilename( + path.join(sliceDir, resource.servePath), + { data: resource.data }), length: resource.data.length, offset: 0, servePath: resource.servePath || undefined @@ -2080,17 +2078,11 @@ _.extend(Package.prototype, { // Output prelink resources _.each(slice.prelinkFiles, function (file) { - var resourcePath = builder.generateFilename( - path.join(sliceDir, file.servePath)); - var data = new Buffer(file.source, 'utf8'); - - builder.write(resourcePath, { - data: data - }); - var resource = { type: 'prelink', - file: resourcePath, + file: builder.writeToGeneratedFilename( + path.join(sliceDir, file.servePath), + { data: new Buffer(file.source, 'utf8') }), length: data.length, offset: 0, servePath: file.servePath || undefined @@ -2098,25 +2090,20 @@ _.extend(Package.prototype, { if (file.sourceMap) { // Write the source map. - var mapFilename = builder.generateFilename( - path.join(sliceDir, file.servePath + '.map')); - resource.sourceMap = mapFilename; - builder.write(mapFilename, { - data: new Buffer(file.sourceMap.toString(), 'utf8') - }); + resource.sourceMap = builder.writeToGeneratedFilename( + path.join(sliceDir, file.servePath + '.map'), + { data: new Buffer(file.sourceMap.toString(), 'utf8') } + ); // Now write the sources themselves. resource.sources = {}; _.each(file.sources, function (x, pathForSourceMap) { - var savedFilename = builder.generateFilename( - path.join(sliceDir, 'sources', x.sourcePath)); - builder.write(savedFilename, { - data: x.source - }); resource.sources[pathForSourceMap] = { package: x.package, sourcePath: x.sourcePath, - source: savedFilename + source: builder.writeToGeneratedFilename( + path.join(sliceDir, 'sources', x.sourcePath), + { data: x.source }) }; }); } From 7ef61492da14495052f0a3a4e41413e05d425173 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 9 Jul 2013 11:07:13 -0700 Subject: [PATCH 23/49] refactor disk layout of client programs to get rid of static/static_cacheable This introduces some hacks into how asset dirs work. Will fix later. Might have broken server assets. Who knows. --- packages/webapp/webapp_server.js | 8 ++--- tools/bundler.js | 56 +++++++++++++------------------- tools/packages.js | 3 +- 3 files changed, 28 insertions(+), 39 deletions(-) diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js index 90094d1261..6e2b9a8abd 100644 --- a/packages/webapp/webapp_server.js +++ b/packages/webapp/webapp_server.js @@ -229,15 +229,15 @@ var runWebAppServer = function () { next(); return; } - var path = connect.utils.parseUrl(req).pathname; + var pathname = connect.utils.parseUrl(req).pathname; try { - path = decodeURIComponent(path); + pathname = decodeURIComponent(pathname); } catch (e) { next(); return; } - if (!_.has(staticFiles, path)) { + if (!_.has(staticFiles, pathname)) { next(); return; } @@ -246,7 +246,7 @@ var runWebAppServer = function () { // 'send' and yield to the event loop, we never call another handler with // 'next'. - var info = staticFiles[path]; + var info = staticFiles[pathname]; // Cacheable files are files that should never change. Typically // named by their hash (eg meteor bundled js and css files). diff --git a/tools/bundler.js b/tools/bundler.js index 1e862c7fa9..10a2a7bbb2 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -380,6 +380,8 @@ _.extend(File.prototype, { // static/packages specific to this package. Application assets (e.g. those // inside private/) go in static/app/. // XXX same hack as above + // XXX XXX is this all still true? + // XXX rename static -> assets on server var bundlePath; if (relPath.match(/^packages\//)) { var dir = path.dirname(relPath); @@ -496,11 +498,6 @@ _.extend(Target.prototype, { self._addCacheBusters("js"); self._addCacheBusters("css"); } - - // XXX extra thing we have to do on the client. could this move - // into ClientTarget.write()? - if (self.assignTargetPaths) - self.assignTargetPaths(); }, // Determine the packages to load, create Slices for @@ -645,16 +642,17 @@ _.extend(Target.prototype, { cacheable: false }); + var relPath; + if (resource.type === "static" && isNative) + relPath = path.join("static", resource.servePath); + else { + relPath = stripLeadingSlash(resource.servePath); + } + f.setTargetPathFromRelPath(relPath); + if (isBrowser) { f.setUrlFromRelPath(resource.servePath); } else if (isNative) { - var relPath; - if (resource.type === "static") - relPath = path.join("static", resource.servePath); - else { - relPath = stripLeadingSlash(resource.servePath); - } - f.setTargetPathFromRelPath(relPath); if (resource.type === "js") f.setStaticDirectory(relPath, resource.staticDirectory); } @@ -789,8 +787,13 @@ _.extend(Target.prototype, { var f = new File({ sourcePath: absPath }); if (setUrl) f.setUrlFromRelPath(assetPath); + // XXX why is this separate from _emitResources ? + // XXX fix up server static resources + var relPath = assetDir.useSubDirectory + ? path.join('static', 'app', assetPath) + : assetPath; if (setTargetPath) - f.setTargetPathFromRelPath(path.join('/static', 'app', assetPath)); + f.setTargetPathFromRelPath(relPath); self.dependencyInfo.files[absPath] = f.hash(); self.static.push(f); }); @@ -837,22 +840,6 @@ _.extend(ClientTarget.prototype, { self.css[0].setUrlToHash(".css"); }, - assignTargetPaths: function () { - var self = this; - _.each(["js", "css", "static"], function (type) { - _.each(self[type], function (file) { - if (! file.targetPath) { - if (! file.url) - throw new Error("Client file with no URL?"); - - var parts = file.url.replace(/\?.*$/, '').split('/').slice(1); - parts.unshift(file.cacheable ? "static_cacheable" : "static"); - file.targetPath = path.sep + path.join.apply(path, parts); - } - }); - }); - }, - generateHtmlBoilerplate: function () { var self = this; @@ -911,7 +898,7 @@ _.extend(ClientTarget.prototype, { if (file.sourceMap) { manifestItem.sourceMap = builder.writeToGeneratedFilename( - stripLeadingSlash(file.targetPath + '.map'), { + file.targetPath + '.map', { data: new Buffer(file.sourceMap.toString(), 'utf8') }); @@ -1637,9 +1624,8 @@ exports.bundle = function (appDir, outputPath, options) { assetDirs = assetDirs || []; var clientAssetDirs = getValidAssetDirs(assetDirs, { exclude: ignoreFiles, - setUrl: true - // No need to set targetPath when the asset dir is added; - // the target path will be set later in assignTargetPaths. + setUrl: true, + setTargetPath: true }); client.make({ @@ -1671,9 +1657,11 @@ exports.bundle = function (appDir, outputPath, options) { assetDirs = assetDirs || []; var serverAssetDirs = getValidAssetDirs(assetDirs, { exclude: ignoreFiles, - setTargetPath: true // We need to set the target path when the asset dir is added, // because the target path comes from the asset's path. + setTargetPath: true, + // XXX this is a hack, re-assess how the subdirs are named + useSubDirectory: true }); var targetOptions = { library: library, diff --git a/tools/packages.js b/tools/packages.js index eb8d3cd1c4..8850919487 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -2078,11 +2078,12 @@ _.extend(Package.prototype, { // Output prelink resources _.each(slice.prelinkFiles, function (file) { + var data = new Buffer(file.source, 'utf8'); var resource = { type: 'prelink', file: builder.writeToGeneratedFilename( path.join(sliceDir, file.servePath), - { data: new Buffer(file.source, 'utf8') }), + { data: data }), length: data.length, offset: 0, servePath: file.servePath || undefined From 31e560e8ca3a780f8d526e368470b29f5bbd3088 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 9 Jul 2013 11:44:01 -0700 Subject: [PATCH 24/49] serve source maps (but not sources) they don't seem to actually work in chrome yet --- packages/webapp/webapp_server.js | 37 ++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js index 6e2b9a8abd..f2373992d4 100644 --- a/packages/webapp/webapp_server.js +++ b/packages/webapp/webapp_server.js @@ -5,6 +5,7 @@ var http = Npm.require("http"); var os = Npm.require("os"); var path = Npm.require("path"); var url = Npm.require("url"); +var crypto = Npm.require("crypto"); var connect = Npm.require('connect'); var optimist = Npm.require('optimist'); @@ -50,6 +51,12 @@ var initKeepalive = function () { }; +var sha1 = function (contents) { + var hash = crypto.createHash('sha1'); + hash.update(contents); + return hash.digest('hex'); +}; + // #BrowserIdentification // // We have multiple places that want to identify the browser: the @@ -218,8 +225,29 @@ var runWebAppServer = function () { var staticFiles = {}; _.each(clientJson.manifest, function (item) { - if (item.url && item.where === "client") - staticFiles[url.parse(item.url).pathname] = item; + if (item.url && item.where === "client") { + var staticFile = { + path: item.path, + cacheable: item.cacheable + }; + + // Serve the source map too, under a hashed URL. Note that the hash is + // based on item.url which contains the file's hash, so this should change + // when the file changes and thus be cacheable. The URL ends with a slash, + // so that source files referenced from the source map with relative URLs + // are resolved under it. + if (item.sourceMap) { + var sourceMapRootUrl = "/_sources/" + sha1(item.url) + "/"; + // Register the source map itself to be served. + staticFiles[sourceMapRootUrl] = { + path: item.sourceMap, + cacheable: true + }; + // Send the SourceMap header when the source file is served. + staticFile.sourceMap = sourceMapRootUrl; + } + staticFiles[url.parse(item.url).pathname] = staticFile; + } }); // Serve static files from the manifest. @@ -263,6 +291,11 @@ var runWebAppServer = function () { ? 1000 * 60 * 60 * 24 * 365 : 1000 * 60 * 60 * 24; + // Tell the client where to find the source map for this file. + if (info.sourceMap) { + res.setHeader('SourceMap', info.sourceMap); + } + send(req, path.join(clientDir, info.path)) .maxage(maxAge) .hidden(true) // if we specified a dotfile in the manifest, serve it From c329ebf90d953429503e286d7d7205aa284de21e Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 9 Jul 2013 12:45:15 -0700 Subject: [PATCH 25/49] Serve sources as well, and use X-SourceMap header. Source maps now work in Chrome, if you enable them (dev tools -> gears button -> enable source maps). I can't get them to work in FF 24 though. --- packages/webapp/webapp_server.js | 13 ++++++++++++- tools/bundler.js | 11 ++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js index f2373992d4..8e6265c524 100644 --- a/packages/webapp/webapp_server.js +++ b/packages/webapp/webapp_server.js @@ -245,6 +245,15 @@ var runWebAppServer = function () { }; // Send the SourceMap header when the source file is served. staticFile.sourceMap = sourceMapRootUrl; + + // Now register all the sources from this source map to be served, under + // the source map's URL. + _.each(item.sources, function (x, pathForSourceMap) { + staticFiles[sourceMapRootUrl + pathForSourceMap] = { + path: x.source, + cacheable: true + }; + }); } staticFiles[url.parse(item.url).pathname] = staticFile; } @@ -293,7 +302,9 @@ var runWebAppServer = function () { // Tell the client where to find the source map for this file. if (info.sourceMap) { - res.setHeader('SourceMap', info.sourceMap); + // This should just be SourceMap, but slightly more browsers support the + // older X-SourceMap. + res.setHeader('X-SourceMap', info.sourceMap); } send(req, path.join(clientDir, info.path)) diff --git a/tools/bundler.js b/tools/bundler.js index 10a2a7bbb2..51b4ecb350 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -902,7 +902,16 @@ _.extend(ClientTarget.prototype, { data: new Buffer(file.sourceMap.toString(), 'utf8') }); - // XXX write sources too + manifestItem.sources = {}; + _.each(file.sources, function (x, pathForSourceMap) { + manifestItem.sources[pathForSourceMap] = { + package: x.package, + sourcePath: x.sourcePath, + source: builder.writeToGeneratedFilename( + path.join('sources', pathForSourceMap), + { data: x.source }) + }; + }); } manifest.push(manifestItem); From af01be5004cb3d652f8b7e08458abd310dda2f43 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 9 Jul 2013 13:34:39 -0700 Subject: [PATCH 26/49] Comment about various browser's support for source map headers. --- packages/webapp/webapp_server.js | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js index 8e6265c524..66af9c24d1 100644 --- a/packages/webapp/webapp_server.js +++ b/packages/webapp/webapp_server.js @@ -302,8 +302,34 @@ var runWebAppServer = function () { // Tell the client where to find the source map for this file. if (info.sourceMap) { - // This should just be SourceMap, but slightly more browsers support the - // older X-SourceMap. + // This should just be SourceMap, but slightly more versions of Chrome + // support the older X-SourceMap. + // + // To figure out if your version of Chrome should support SourceMap, + // - go to chrome://version. Let's say the Chrome version is + // 28.0.1500.71 and the Blink version is 537.36 (@153022) + // - go to http://src.chromium.org/viewvc/blink/branches/chromium/1500/Source/core/inspector/InspectorPageAgent.cpp?view=log + // where the "1500" is the third part of your Chrome version + // - find the first revision that is no greater than the "153022" + // number. That's probably the first one and it probably has + // a message of the form "Branch 1500 - blink@r149738" + // - If *that* revision number (149738) is at least 151755, + // then Chrome should support SourceMap (not just X-SourceMap) + // (The change is https://codereview.chromium.org/15832007) + // + // You also need to enable source maps in Chrome: open dev tools, click + // the gear in the bottom right corner, and select "enable source maps". + // + // Firefox 23+ supports source maps (and they are on by default in 24+), + // but doesn't support either header yet: + // https://bugzilla.mozilla.org/show_bug.cgi?id=765993 + // We could make FF work by adding a comment to the end of the source map + // file. But that would require doing one of the following: + // - determining the source map URL at bundle time instead of here + // - writing to the source directory + // - not using the send module (or hacking it to allow a footer) + // None of these alternatives are great, so for now we just hope that + // FF will implement one of the headers soon. res.setHeader('X-SourceMap', info.sourceMap); } From 040d97d6c0fdd9ab3886545721cfdc65fcfab000 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 9 Jul 2013 13:41:34 -0700 Subject: [PATCH 27/49] Fix production mode bundles. --- tools/bundler.js | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/bundler.js b/tools/bundler.js index 51b4ecb350..366ea23e4a 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -336,6 +336,7 @@ _.extend(File.prototype, { var self = this; self.url = "/" + self.hash() + suffix; self.cacheable = true; + self.targetPath = self.hash() + suffix; }, // Append "?" to the URL and mark the file as cacheable. From d823e57049b5697b68934a5ebb10f9499d1379ca Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 9 Jul 2013 14:10:00 -0700 Subject: [PATCH 28/49] Rip @unit out of Meteor. It's not used, it doesn't fully work yet, it's incompatible with the static analysis, and it will make implementing CoffeeScript source maps significantly more difficult. --- .../plugin/compile-coffeescript.js | 2 +- tools/linker.js | 285 ++++++------------ tools/packages.js | 2 +- 3 files changed, 94 insertions(+), 195 deletions(-) diff --git a/packages/coffeescript/plugin/compile-coffeescript.js b/packages/coffeescript/plugin/compile-coffeescript.js index 58fc659788..1d09712744 100644 --- a/packages/coffeescript/plugin/compile-coffeescript.js +++ b/packages/coffeescript/plugin/compile-coffeescript.js @@ -102,7 +102,7 @@ var handler = function (compileStep) { sourcePath: compileStep.inputPath, data: output, lineForLine: false, - linkerUnitTransform: function (source, exports) { + linkerFileTransform: function (source, exports) { return addSharedHeader(stripExportedVars(source, exports)); } }); diff --git a/tools/linker.js b/tools/linker.js index e3d2b631ad..bc6d5bb00e 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -129,9 +129,8 @@ _.extend(Module.prototype, { // Otherwise.. - // Find the maximum line length. The extra three are for the - // comments that will be emitted when we skip a unit. - var sourceWidth = _.max([68, self.maxLineLength(120 - 2)]) + 3; + // Find the maximum line length. + var sourceWidth = _.max([68, self.maxLineLength(120 - 2)]); // Prologue var chunks = []; @@ -168,9 +167,7 @@ _.extend(Module.prototype, { var exports = {}; _.each(self.files, function (file) { - _.each(file.units, function (unit) { - _.extend(exports, unit.exports); - }); + _.extend(exports, file.exports); }); return _.union(_.keys(exports), self.forceExport); @@ -241,14 +238,10 @@ var File = function (inputFile, module) { // should line and column be included in errors? self.includePositionInErrors = inputFile.includePositionInErrors; - // The individual @units in the file. Array of Unit. Concatenating - // the source of each unit, in order, will give self.source. - self.units = []; - // A function which transforms the source code once all exports are // known. (eg, for CoffeeScript.) - self.linkerUnitTransform = - inputFile.linkerUnitTransform || function (source, exports) { + self.linkerFileTransform = + inputFile.linkerFileTransform || function (source, exports) { return source; }; @@ -258,23 +251,52 @@ var File = function (inputFile, module) { // The Module containing this file. self.module = module; - self._unitize(); + // symbols mentioned in @export, @require, @provide, or @weak + // directives. each is a map from the symbol (given as a string) to + // true. (only @export is actually implemented) + self.exports = {}; + self.requires = {}; + self.provides = {}; + self.weaks = {}; + + self._scanForComments(); }; _.extend(File.prototype, { - // Return the union of the global references in all of the units in - // this file that we are actually planning to use. Array of string. + // Return the globals in this file as an array of symbol names. For + // example: if the code references 'Foo.bar.baz' and 'Quux', and + // neither are declared in a scope enclosing the point where they're + // referenced, then globalReferences would include ["Foo", "Quux"]. computeGlobalReferences: function () { var self = this; - var globalReferences = []; - _.each(self.units, function (unit) { - if (unit.include) - globalReferences = globalReferences.concat(unit.computeGlobalReferences()); - }); - return globalReferences; + var jsAnalyze = self.module.jsAnalyze; + // If we don't have a JSAnalyze object, we probably are the js-analyze + // package itself. Assume we have no global references. At the module level, + // we'll assume that exports are global references. + if (!jsAnalyze) + return []; + + try { + return _.keys(jsAnalyze.findAssignedGlobals(self.source)); + } catch (e) { + if (!e.$ParseError) + throw e; + buildmessage.error(e.description, { + file: self.sourcePath, + line: self.includePositionInErrors ? e.lineNumber : null, + column: self.includePositionInErrors ? e.column : null, + downcase: true + }); + + // Recover by pretending that this file is empty (which + // includes replacing its source code with '' in the output) + self.source = ""; + return []; + } }, + // Relative path to use in source maps to indicate this file. No // leading slash. _pathForSourceMap: function () { @@ -311,8 +333,8 @@ _.extend(File.prototype, { // The newline after the source closes a '//' comment. if (options.preserveLineNumbers) { // Ugly version - // XXX XXX need to propagate source maps through linkerUnitTransform! - var body = self.linkerUnitTransform(self.source, options.exports); + // XXX XXX need to propagate source maps through linkerFileTransform! + var body = self.linkerFileTransform(self.source, options.exports); return new sourcemap.SourceNode(null, null, null, [ self.bare ? "" : "(function(){", new sourcemap.SourceNode(1, 0, self._pathForSourceMap(), @@ -354,37 +376,26 @@ _.extend(File.prototype, { // comments, because you have to do something different if you're // already inside a comment. var num = 1; - _.each(self.units, function (unit) { - var unitSource = self.linkerUnitTransform(unit.source, options.exports); - var lines = unitSource.split('\n'); + var transformedSource = self.linkerFileTransform( + self.source, options.exports); + var lines = transformedSource.split('\n'); - // There are probably ways to make a more compact source - // map. For example, for an included unit, the only change we - // make is to append a comment, so we can probably emit one - // mapping for the whole unit. And for a non-included unit, we - // can probably tolerate mapping it inexactly or not at all - // (since it's in a comment.) For the moment, we'll do it by the - // book just to see how it goes. + // There are probably ways to make a more compact source map. For example, + // the only change we make is to append a comment, so we can probably emit + // one mapping for the whole file. For the moment, we'll do it by the book + // just to see how it goes. + _.each(lines, function (line) { + var suffix = "\n"; - _.each(lines, function (line) { - var prefix = "", suffix = "\n"; + if (line.length <= width) { + suffix = padding.slice(line.length, width) + " // " + num + "\n"; + } - if (! unit.include) { - prefix = "// "; - } + chunks.push(new sourcemap.SourceNode(num, 0, self._pathForSourceMap(), + line)); + chunks.push(suffix); - var lengthWithPrefix = line.length + prefix.length; - if (lengthWithPrefix <= width) { - suffix = padding.slice(lengthWithPrefix, width) + " // " + num + "\n"; - } - - chunks.push(prefix); - chunks.push(new sourcemap.SourceNode(num, 0, self._pathForSourceMap(), - line)); - chunks.push(suffix); - - num++; - }); + num++; }); // Footer @@ -412,157 +423,45 @@ _.extend(File.prototype, { return null; }, - // Split file and populate self.units - // XXX it is an error to declare a @unit not at toplevel (eg, inside a - // function or object..) We don't detect this but we might have to to - // give an acceptable user experience.. - _unitize: function () { + // Scan for @export, etc. + _scanForComments: function () { var self = this; var lines = self.source.split("\n"); - var buf = ""; - var unit = new Unit( - null, true, self, self.includePositionInErrors ? 0 : null); - self.units.push(unit); - var lineCount = 0; _.each(lines, function (line) { var commentBody = self._getSingleLineCommentBody(line); + if (!commentBody) + return; - if (commentBody) { - // XXX overly permissive. should detect errors - var match = /^@unit(?:\s+(\S+))?$/.exec(commentBody); - if (match) { - unit.source = buf; - buf = line; - unit = new Unit(match[1] || null, false, self, - self.includePositionInErrors ? lineCount : null); - self.units.push(unit); - lineCount++; - return; - } + // XXX overly permissive. should detect errors + var match = /^@(export|require|provide|weak)(\s+.*)$/.exec(commentBody); + if (match) { + var what = match[1]; + var symbols = _.map(match[2].split(/,/), function (s) { + return s.trim(); + }); - // XXX overly permissive. should detect errors - match = /^@(export|require|provide|weak)(\s+.*)$/.exec(commentBody); - if (match) { - var what = match[1]; - var symbols = _.map(match[2].split(/,/), function (s) { - return s.trim(); + var badSymbols = _.reject(symbols, function (s) { + // XXX should be unicode-friendlier + return s.match(/^([_$a-zA-Z][_$a-zA-Z0-9]*)(\.[_$a-zA-Z][_$a-zA-Z0-9]*)*$/); + }); + if (!_.isEmpty(badSymbols)) { + buildmessage.error("bad symbols for @" + what + ": " + + JSON.stringify(badSymbols), + { file: self.sourcePath }); + // recover by ignoring + } else if (self.module.noExports && what === "export") { + buildmessage.error("@export not allowed in this slice", + { file: self.sourcePath }); + // recover by ignoring + } else { + _.each(symbols, function (s) { + if (s.length) + self[what + "s"][s] = true; }); - - var badSymbols = _.reject(symbols, function (s) { - // XXX should be unicode-friendlier - return s.match(/^([_$a-zA-Z][_$a-zA-Z0-9]*)(\.[_$a-zA-Z][_$a-zA-Z0-9]*)*$/); - }); - if (!_.isEmpty(badSymbols)) { - buildmessage.error("bad symbols for @" + what + ": " + - JSON.stringify(badSymbols), - { file: self.sourcePath }); - // recover by ignoring - } else if (self.module.noExports && what === "export") { - buildmessage.error("@export not allowed in this slice", - { file: self.sourcePath }); - // recover by ignoring - } else { - _.each(symbols, function (s) { - if (s.length) - unit[what + "s"][s] = true; - }); - } - - /* fall through */ } } - - if (lineCount !== 0) - buf += "\n"; - lineCount++; - buf += line; }); - unit.source = buf; - } -}); - -/////////////////////////////////////////////////////////////////////////////// -// Unit -/////////////////////////////////////////////////////////////////////////////// - -var Unit = function (name, mandatory, file, lineOffset) { - var self = this; - - // name of the unit, or null if none provided - self.name = name; - - // source code for this unit (a string) - self.source = null; - - // true if this unit is to always be included - self.mandatory = !! mandatory; - - // true if we should include this unit in the linked output - self.include = self.mandatory; - - // The File containing the unit. - self.file = file; - - // offset of 'self.source' in the original input file, in whole - // lines (partial lines are not supported.) Used to generate correct - // line number information in error messages. Set to null to omit - // line/column information (you'll need to do this, for, eg, - // coffeescript output, given that we don't have sourcemaps here - // yet.) - self.lineOffset = lineOffset; - - // symbols mentioned in @export, @require, @provide, or @weak - // directives. each is a map from the symbol (given as a string) to - // true. - self.exports = {}; - self.requires = {}; - self.provides = {}; - self.weaks = {}; -}; - -_.extend(Unit.prototype, { - // Return the globals in unit file as an array of symbol names. For - // example: if the code references 'Foo.bar.baz' and 'Quux', and - // neither are declared in a scope enclosing the point where they're - // referenced, then globalReferences would include ["Foo", "Quux"]. - // - // XXX Doing this at the unit level means that we need to also look - // for var declarations in various units, and use them to create - // a graph of unit dependencies such that in: - // // @unit X - // var A; - // // @unit Y - // A = 5; - // including Y requires including X. Since we don't do that, @unit - // is currently broken. It's also unused and undocumented :) - computeGlobalReferences: function () { - var self = this; - - var jsAnalyze = self.file.module.jsAnalyze; - // If we don't have a JSAnalyze object, we probably are the js-analyze - // package itself. Assume we have no global references. At the module level, - // we'll assume that exports are global references. - if (!jsAnalyze) - return []; - - try { - return _.keys(jsAnalyze.findAssignedGlobals(self.source)); - } catch (e) { - if (!e.$ParseError) - throw e; - buildmessage.error(e.description, { - file: self.file.sourcePath, - line: self.lineOffset === null ? null : e.lineNumber + self.lineOffset, - column: self.lineOffset === null ? null : e.column, - downcase: true - }); - - // Recover by pretending that this unit is empty (which - // includes replacing its source code with '' in the output) - self.source = ""; - return []; - } } }); @@ -593,9 +492,9 @@ _.extend(Unit.prototype, { // - includePositionInErrors: true to include line and column // information in errors. set to false if, eg, this is the output // of coffeescript. (XXX replace with real sourcemaps) -// - linkerUnitTransform: if given, this function will be called -// when the module is being linked with the source of the unit -// and an array of the exports of the module; the unit's source will +// - linkerFileTransform: if given, this function will be called +// when the module is being linked with the source of the file +// and an array of the exports of the module; the file's source will // be replaced by what the function returns. // // forceExport: an array of symbols (as dotted strings) to force the diff --git a/tools/packages.js b/tools/packages.js index 8850919487..9616b6db1e 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -414,7 +414,7 @@ _.extend(Slice.prototype, { sourcePath: options.sourcePath, servePath: path.join(self.pkg.serveRoot, options.path), includePositionInErrors: options.lineForLine, - linkerUnitTransform: options.linkerUnitTransform, + linkerFileTransform: options.linkerFileTransform, bare: !!options.bare }); }, From 4e18439ae6b69663c73b92674296d7272a20020f Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 9 Jul 2013 14:59:14 -0700 Subject: [PATCH 29/49] Source maps for coffeescript. --- .../compileCoffeescript/npm-shrinkwrap.json | 8 ++ packages/coffeescript/package.js | 2 +- .../plugin/compile-coffeescript.js | 95 +++++++++++---- tools/linker.js | 110 +++++++++++++----- tools/packages.js | 17 ++- 5 files changed, 177 insertions(+), 55 deletions(-) diff --git a/packages/coffeescript/.npm/plugin/compileCoffeescript/npm-shrinkwrap.json b/packages/coffeescript/.npm/plugin/compileCoffeescript/npm-shrinkwrap.json index ae2363c418..5df65e6c16 100644 --- a/packages/coffeescript/.npm/plugin/compileCoffeescript/npm-shrinkwrap.json +++ b/packages/coffeescript/.npm/plugin/compileCoffeescript/npm-shrinkwrap.json @@ -2,6 +2,14 @@ "dependencies": { "coffee-script": { "version": "1.6.3" + }, + "source-map": { + "version": "0.1.24", + "dependencies": { + "amdefine": { + "version": "0.0.5" + } + } } } } diff --git a/packages/coffeescript/package.js b/packages/coffeescript/package.js index f7db750b0c..2a13f962fb 100644 --- a/packages/coffeescript/package.js +++ b/packages/coffeescript/package.js @@ -8,7 +8,7 @@ Package._transitional_registerBuildPlugin({ sources: [ 'plugin/compile-coffeescript.js' ], - npmDependencies: {"coffee-script": "1.6.3"} + npmDependencies: {"coffee-script": "1.6.3", "source-map": "0.1.24"} }); Package.on_test(function (api) { diff --git a/packages/coffeescript/plugin/compile-coffeescript.js b/packages/coffeescript/plugin/compile-coffeescript.js index 1d09712744..5af416a399 100644 --- a/packages/coffeescript/plugin/compile-coffeescript.js +++ b/packages/coffeescript/plugin/compile-coffeescript.js @@ -2,6 +2,7 @@ var fs = Npm.require('fs'); var path = Npm.require('path'); var coffee = Npm.require('coffee-script'); var _ = Npm.require('underscore'); +var sourcemap = Npm.require('source-map'); var stripExportedVars = function (source, exports) { if (!exports || _.isEmpty(exports)) @@ -28,33 +29,50 @@ var stripExportedVars = function (source, exports) { // XXX relax these assumptions by doing actual JS parsing (eg with jsparse). // I'd do this now, but there's no easy way to "unparse" a jsparse AST. - var foundVarLine = false; - lines = _.map(lines, function (line) { - if (foundVarLine) - return line; + for (var i = 0; i < lines.length; i++) { + var line = lines[i]; var match = /^var (.+)([,;])$/.exec(line); if (!match) - return line; - foundVarLine = true; + continue; // If there's an assignment on this line, we assume that there are ONLY // assignments and that the var we are looking for is not declared. (Part // of our strong assumption about the layout of this code.) if (match[1].indexOf('=') !== -1) - return line; + continue; + + // We want to replace the line with something no shorter, so that all + // records in the source map continue to point at valid + // characters. + var replaceLine = function (x) { + if (x.length >= lines[i].length) { + lines[i] = x; + } else { + lines[i] = x + new Array(1 + (lines[i].length - x.length)).join(' '); + } + }; var vars = match[1].split(', '); vars = _.difference(vars, exports); - if (!_.isEmpty(vars)) - return "var " + vars.join(', ') + match[2]; - // We got rid of all the vars on this line. Drop the whole line if this - // didn't continue to the next line, otherwise keep just the 'var '. - return match[2] === ';' ? '' : 'var'; - }); + if (!_.isEmpty(vars)) { + replaceLine("var " + vars.join(', ') + match[2]); + } else { + // We got rid of all the vars on this line. Drop the whole line if this + // didn't continue to the next line, otherwise keep just the 'var '. + if (match[2] === ';') + replaceLine(''); + else + replaceLine('var'); + } + break; + } + return lines.join('\n'); }; -var addSharedHeader = function (source) { +var addSharedHeader = function (source, sourceMapAsString) { + var sourceMapJSON = JSON.parse(sourceMapAsString); + // We want the symbol "share" to be visible to all CoffeeScript files in the // package (and shared between them), but not visible to JavaScript // files. (That's because we don't want to introduce two competing ways to @@ -73,17 +91,38 @@ var addSharedHeader = function (source) { // If the file begins with "use strict", we need to keep that as the first // statement. - return source.replace(/^(?:(['"])use strict\1;\n)?/, function (match) { - return match + header; + source = source.replace(/^(?:((['"])use strict\2;)\n)?/, function (match, useStrict) { + if (match) { + // There's a "use strict"; we keep this as the first statement and insert + // our header at the end of the line that it's on. This doesn't change + // line numbers or the part of the line that previous may have been + // annotated, so we don't need to update the source map. + return useStrict + " " + header; + } else { + // There's no use strict, so we can just add the header at the very + // beginning. This adds a line to the file, so we update the source map to + // add a single un-annotated line to the beginning. + sourceMapJSON.mappings = ";" + sourceMapJSON.mappings; + return header; + } }); + return { + source: source, + sourceMap: sourcemap.SourceMapGenerator.fromSourceMap( + new sourcemap.SourceMapConsumer(sourceMapJSON)) + }; }; var handler = function (compileStep) { var source = compileStep.read().toString('utf8'); + var outputFile = compileStep.inputPath + ".js"; var options = { bare: true, filename: compileStep.inputPath, - literate: path.extname(compileStep.inputPath) === '.litcoffee' + literate: path.extname(compileStep.inputPath) === '.litcoffee', + sourceMap: true, + generatedFile: "/" + outputFile, // Used in source map. + sourceFiles: [compileStep.pathForSourceMap] // Used in source map. }; try { @@ -97,14 +136,26 @@ var handler = function (compileStep) { ); } + // These are the sources that correspond to the input files in the source map. + var sources = {}; + sources[compileStep.pathForSourceMap] = { + source: new Buffer(source, 'utf8'), + package: compileStep.packageName, + sourcePath: compileStep.inputPath + }; + compileStep.addJavaScript({ - path: compileStep.inputPath + ".js", + path: outputFile, sourcePath: compileStep.inputPath, - data: output, + data: output.js, lineForLine: false, - linkerFileTransform: function (source, exports) { - return addSharedHeader(stripExportedVars(source, exports)); - } + linkerFileTransform: function (source, exports, sourceMap) { + var sourceMapAsString = sourceMap.toString(); + var stripped = stripExportedVars(source, exports); + return addSharedHeader(stripped, sourceMapAsString); + }, + sourceMapAsString: output.v3SourceMap, + sources: sources }); }; diff --git a/tools/linker.js b/tools/linker.js index bc6d5bb00e..27d86d0363 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -115,7 +115,7 @@ _.extend(Module.prototype, { file: file.servePath }); // results has 'code' and 'map' attributes - sources = {}; + var sources = {}; file.addToSourcesSet(sources); return { @@ -146,10 +146,10 @@ _.extend(Module.prototype, { }); var node = new sourcemap.SourceNode(null, null, null, chunks); + var results = node.toStringWithSourceMap({ file: self.combinedServePath }); // results has 'code' and 'map' attributes - return [{ source: results.code, servePath: self.combinedServePath, @@ -241,13 +241,20 @@ var File = function (inputFile, module) { // A function which transforms the source code once all exports are // known. (eg, for CoffeeScript.) self.linkerFileTransform = - inputFile.linkerFileTransform || function (source, exports) { - return source; + inputFile.linkerFileTransform || function (source, exports, sourceMap) { + return {source: source, sourceMap: sourceMap}; }; // If true, don't wrap this individual file in a closure. self.bare = !!inputFile.bare; + // A source map (generated by something like CoffeeScript) for the input file. + // As a SourceMapGenerator. + self.sourceMap = inputFile.sourceMap; + // Information about the source files in a source map. May have come with the + // source map, or we may be building it from scratch. + self.sourcesFromSourceMap = inputFile.sourceMap && inputFile.sources; + // The Module containing this file. self.module = module; @@ -312,11 +319,15 @@ _.extend(File.prototype, { // info about each source file) addToSourcesSet: function (sources) { var self = this; - sources[self._pathForSourceMap()] = { - package: self.module.name, - sourcePath: self.sourcePath, - source: new Buffer(self.source, 'utf8') // XXX encoding - }; + if (self.sourcesFromSourceMap) { + _.extend(sources, self.sourcesFromSourceMap); + } else { + sources[self._pathForSourceMap()] = { + package: self.module.name, + sourcePath: self.sourcePath, + source: new Buffer(self.source, 'utf8') // XXX encoding + }; + } }, // Options: @@ -334,12 +345,26 @@ _.extend(File.prototype, { if (options.preserveLineNumbers) { // Ugly version // XXX XXX need to propagate source maps through linkerFileTransform! - var body = self.linkerFileTransform(self.source, options.exports); + var bodyWithMap = self.linkerFileTransform(self.source, options.exports, + self.sourceMap); + self.source = bodyWithMap.source; + var mapNode; + if (bodyWithMap.sourceMap) { + mapNode = sourcemap.SourceNode.fromStringWithSourceMap( + self.source, + new sourcemap.SourceMapConsumer( + bodyWithMap.sourceMap.toString())); + } else { + mapNode = new sourcemap.SourceNode(1, 0, self._pathForSourceMap(), + self.source); + } + return new sourcemap.SourceNode(null, null, null, [ self.bare ? "" : "(function(){", - new sourcemap.SourceNode(1, 0, self._pathForSourceMap(), - body), - body.length && body[body.length - 1] !== '\n' ? "\n" : "", + mapNode, + (self.source.length + && bodyWithMap.source[self.source.length - 1] !== '\n' + ? "\n" : ""), self.bare ? "" : "\n})();\n" ]); } @@ -370,39 +395,62 @@ _.extend(File.prototype, { header += spacer + divider + blankLine; chunks.push(header); + var transformedSourceWithMap = self.linkerFileTransform( + self.source, options.exports, self.sourceMap); + self.source = transformedSourceWithMap.source; + // Code, with line numbers // You might prefer your line numbers at the beginning of the // line, with /* .. */. Well, that requires parsing the source for // comments, because you have to do something different if you're // already inside a comment. - var num = 1; - var transformedSource = self.linkerFileTransform( - self.source, options.exports); - var lines = transformedSource.split('\n'); + var numberifyLines = function (f) { + var num = 1; + var lines = self.source.split('\n'); + _.each(lines, function (line) { + var suffix = "\n"; + + if (line.length <= width) { + suffix = padding.slice(line.length, width) + " // " + num + "\n"; + } + f(line, suffix); + num++; + }); + }; + + var num = 1; + var lines = self.source.split('\n'); + + if (transformedSourceWithMap.sourceMap) { + var buf = ""; + numberifyLines(function (line, suffix) { + buf += line; + buf += suffix; + }); + // The existing source map is valid because all we're doing is adding + // things to the end of lines, which doesn't affect the source map. + chunks.push(sourcemap.SourceNode.fromStringWithSourceMap( + self.source, new sourcemap.SourceMapConsumer(transformedSourceWithMap.sourceMap.toString()))); + } else { // There are probably ways to make a more compact source map. For example, // the only change we make is to append a comment, so we can probably emit // one mapping for the whole file. For the moment, we'll do it by the book // just to see how it goes. - _.each(lines, function (line) { - var suffix = "\n"; - - if (line.length <= width) { - suffix = padding.slice(line.length, width) + " // " + num + "\n"; - } - - chunks.push(new sourcemap.SourceNode(num, 0, self._pathForSourceMap(), - line)); - chunks.push(suffix); - - num++; - }); + numberifyLines(function (line, suffix) { + chunks.push(new sourcemap.SourceNode(num, 0, self._pathForSourceMap(), + line)); + chunks.push(suffix); + }); + } // Footer if (! self.bare) chunks.push(divider + "\n}).call(this);\n"); - return new sourcemap.SourceNode(null, null, null, chunks); + var node = new sourcemap.SourceNode(null, null, null, chunks); + + return node; }, // If "line" contains nothing but a comment (of either syntax), return the diff --git a/tools/packages.js b/tools/packages.js index 9616b6db1e..748a19076b 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -275,6 +275,8 @@ _.extend(Slice.prototype, { // can ensure that the version of the file that you use is // exactly the one that is recorded in the dependency // information. + // - pathForSourceMap: If this file is to be included in a source map, + // this is the name you should use for it in the map. // - rootOutputPath: on browser targets, for resources such as // stylesheet and static assets, this is the root URL that // will get prepended to the paths you pick for your output @@ -367,6 +369,14 @@ _.extend(Slice.prototype, { inputSize: contents.length, inputPath: relPath, _fullInputPath: absPath, // avoid, see above.. + // XXX duplicates _pathForSourceMap() in linker + pathForSourceMap: ( + self.pkg.name + ? "packages/" + self.pkg.name + "/" + relPath + : "app/" + relPath), + // null if this is an app. intended to be used for the sources + // dictionary for source maps. + packageName: self.pkg.name, rootOutputPath: self.pkg.serveRoot, arch: self.arch, fileOptions: fileOptions, @@ -415,7 +425,12 @@ _.extend(Slice.prototype, { servePath: path.join(self.pkg.serveRoot, options.path), includePositionInErrors: options.lineForLine, linkerFileTransform: options.linkerFileTransform, - bare: !!options.bare + bare: !!options.bare, + sourceMap: (options.sourceMapAsString + && sourcemap.SourceMapGenerator.fromSourceMap( + new sourcemap.SourceMapConsumer( + options.sourceMapAsString))), + sources: options.sources }); }, addAsset: function (options) { From df5d48c9af7846f14717fc5a826bc1f2e0e33355 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Jul 2013 00:48:12 -0700 Subject: [PATCH 30/49] Fix to generation of source maps for concatenated files. --- tools/linker.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/linker.js b/tools/linker.js index 27d86d0363..376a3c79ca 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -414,12 +414,11 @@ _.extend(File.prototype, { if (line.length <= width) { suffix = padding.slice(line.length, width) + " // " + num + "\n"; } - f(line, suffix); + f(line, suffix, num); num++; }); }; - var num = 1; var lines = self.source.split('\n'); if (transformedSourceWithMap.sourceMap) { @@ -437,7 +436,7 @@ _.extend(File.prototype, { // the only change we make is to append a comment, so we can probably emit // one mapping for the whole file. For the moment, we'll do it by the book // just to see how it goes. - numberifyLines(function (line, suffix) { + numberifyLines(function (line, suffix, num) { chunks.push(new sourcemap.SourceNode(num, 0, self._pathForSourceMap(), line)); chunks.push(suffix); From a13f330983eea393e079387c69ef373d1ad4a332 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Jul 2013 00:48:30 -0700 Subject: [PATCH 31/49] All sourceMap fields in data structures are now strings, not generators. --- .../plugin/compile-coffeescript.js | 12 ++++------ tools/bundler.js | 23 ++++++++----------- tools/linker.js | 22 +++++++----------- tools/packages.js | 14 ++++------- 4 files changed, 27 insertions(+), 44 deletions(-) diff --git a/packages/coffeescript/plugin/compile-coffeescript.js b/packages/coffeescript/plugin/compile-coffeescript.js index 5af416a399..2ac1d33f21 100644 --- a/packages/coffeescript/plugin/compile-coffeescript.js +++ b/packages/coffeescript/plugin/compile-coffeescript.js @@ -70,8 +70,8 @@ var stripExportedVars = function (source, exports) { return lines.join('\n'); }; -var addSharedHeader = function (source, sourceMapAsString) { - var sourceMapJSON = JSON.parse(sourceMapAsString); +var addSharedHeader = function (source, sourceMap) { + var sourceMapJSON = JSON.parse(sourceMap); // We want the symbol "share" to be visible to all CoffeeScript files in the // package (and shared between them), but not visible to JavaScript @@ -108,8 +108,7 @@ var addSharedHeader = function (source, sourceMapAsString) { }); return { source: source, - sourceMap: sourcemap.SourceMapGenerator.fromSourceMap( - new sourcemap.SourceMapConsumer(sourceMapJSON)) + sourceMap: JSON.stringify(sourceMapJSON) }; }; @@ -150,11 +149,10 @@ var handler = function (compileStep) { data: output.js, lineForLine: false, linkerFileTransform: function (source, exports, sourceMap) { - var sourceMapAsString = sourceMap.toString(); var stripped = stripExportedVars(source, exports); - return addSharedHeader(stripped, sourceMapAsString); + return addSharedHeader(stripped, sourceMap); }, - sourceMapAsString: output.v3SourceMap, + sourceMap: output.v3SourceMap, sources: sources }); }; diff --git a/tools/bundler.js b/tools/bundler.js index 366ea23e4a..9676eab042 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -263,9 +263,8 @@ var File = function (options) { // disk.) self.sourcePath = options.sourcePath; - // If this file was generated, a sourceMap (provided as a - // sourcemap.SourceMapGenerator) with debugging information. Set with - // setSourceMap. + // If this file was generated, a sourceMap (as a string) with debugging + // information. Set with setSourceMap. self.sourceMap = null; // If sourceMap is set, this is a map from a relative source file @@ -397,13 +396,13 @@ _.extend(File.prototype, { }); }, - // Set a source map for this File. sourceMap is given as a - // sourcemap.SourceMapGenerator. See self.sources for the format of sources. + // Set a source map for this File. sourceMap is given as a string. See + // self.sources for the format of sources. setSourceMap: function (sourceMap, sources) { var self = this; - if (!(sourceMap instanceof sourcemap.SourceMapGenerator)) - throw new Error("sourceMap must be given as a SourceMapGenerator"); + if (typeof sourceMap !== "string") + throw new Error("sourceMap must be given as a string"); self.sourceMap = sourceMap; self.sources = sources || {}; } @@ -900,7 +899,7 @@ _.extend(ClientTarget.prototype, { if (file.sourceMap) { manifestItem.sourceMap = builder.writeToGeneratedFilename( file.targetPath + '.map', { - data: new Buffer(file.sourceMap.toString(), 'utf8') + data: new Buffer(file.sourceMap, 'utf8') }); manifestItem.sources = {}; @@ -957,7 +956,7 @@ var JsImage = function () { // - source: JS source code to load, as a string // - nodeModulesDirectory: a NodeModulesDirectory indicating which // directory should be searched by Npm.require() - // - sourceMap: if set, source map for this code, as a SourceMapGenerator + // - sourceMap: if set, source map for this code, as a string // - sources: map from relative path in source map to object with // keys 'source' (a Buffer), 'package', 'sourcePath' // note: this can't be called `load` at it would shadow `load()` @@ -1122,7 +1121,7 @@ _.extend(JsImage.prototype, { // XXX this code is very similar to saveAsUnipackage. loadItem.sourceMap = builder.writeToGeneratedFilename( item.targetPath + '.map', - { data: new Buffer(item.sourceMap.toString(), 'utf8') } + { data: new Buffer(item.sourceMap, 'utf8') } ); // Now write the sources themselves. @@ -1206,10 +1205,8 @@ JsImage.readFromDisk = function (controlFilePath) { if (item.sourceMap) { // XXX this is the same code as initFromUnipackage rejectBadPath(item.sourceMap); - var rawSourceMap = fs.readFileSync( + loadItem.sourceMap = fs.readFileSync( path.join(dir, item.sourceMap), 'utf8'); - loadItem.sourceMap = sourcemap.SourceMapGenerator.fromSourceMap( - new sourcemap.SourceMapConsumer(rawSourceMap)); if (item.sources) { loadItem.sources = {}; _.each(item.sources, function (x, pathForSourceMap) { diff --git a/tools/linker.js b/tools/linker.js index 376a3c79ca..d03a6d21b7 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -121,7 +121,7 @@ _.extend(Module.prototype, { return { source: results.code, servePath: file.servePath, - sourceMap: results.map, + sourceMap: results.map.toString(), sources: sources }; }); @@ -153,7 +153,7 @@ _.extend(Module.prototype, { return [{ source: results.code, servePath: self.combinedServePath, - sourceMap: results.map, + sourceMap: results.map.toString(), sources: sources }]; }, @@ -249,7 +249,6 @@ var File = function (inputFile, module) { self.bare = !!inputFile.bare; // A source map (generated by something like CoffeeScript) for the input file. - // As a SourceMapGenerator. self.sourceMap = inputFile.sourceMap; // Information about the source files in a source map. May have come with the // source map, or we may be building it from scratch. @@ -351,9 +350,7 @@ _.extend(File.prototype, { var mapNode; if (bodyWithMap.sourceMap) { mapNode = sourcemap.SourceNode.fromStringWithSourceMap( - self.source, - new sourcemap.SourceMapConsumer( - bodyWithMap.sourceMap.toString())); + self.source, new sourcemap.SourceMapConsumer(bodyWithMap.sourceMap)); } else { mapNode = new sourcemap.SourceNode(1, 0, self._pathForSourceMap(), self.source); @@ -430,7 +427,8 @@ _.extend(File.prototype, { // The existing source map is valid because all we're doing is adding // things to the end of lines, which doesn't affect the source map. chunks.push(sourcemap.SourceNode.fromStringWithSourceMap( - self.source, new sourcemap.SourceMapConsumer(transformedSourceWithMap.sourceMap.toString()))); + self.source, + new sourcemap.SourceMapConsumer(transformedSourceWithMap.sourceMap))); } else { // There are probably ways to make a more compact source map. For example, // the only change we make is to append a comment, so we can probably emit @@ -569,7 +567,7 @@ _.extend(File.prototype, { // Output is an object with keys: // - files: is an array of output files in the same format as inputFiles // - EXCEPT THAT, for now, sourcePath is omitted and is replaced with -// sourceMap (a SourceMapGenerator) and sources (map to keys 'package', +// sourceMap (a string) and sources (map to keys 'package', // 'sourcePath', 'path') similar to self.resources in a Slice (XXX) // - exports: the exports, as a list of string ('Foo', 'Thing.Stuff', etc) var prelink = function (options) { @@ -642,14 +640,10 @@ var link = function (options) { var ret = []; _.each(options.prelinkFiles, function (file) { if (file.sourceMap) { - // XXX we read this as a string (in initFromUnipackage), then converted to - // Consumer and then to Generator. It's wasteful to then convert back to - // string and to consumer. var node = new sourcemap.SourceNode(null, null, null, [ header, sourcemap.SourceNode.fromStringWithSourceMap( - file.source, - new sourcemap.SourceMapConsumer(file.sourceMap.toString())), + file.source, new sourcemap.SourceMapConsumer(file.sourceMap)), footer ]); var results = node.toStringWithSourceMap({ @@ -658,7 +652,7 @@ var link = function (options) { ret.push({ source: results.code, servePath: file.servePath, - sourceMap: results.map, + sourceMap: results.map.toString(), sources: file.sources }); } else { diff --git a/tools/packages.js b/tools/packages.js index 748a19076b..59b7b922d6 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -181,8 +181,7 @@ var Slice = function (pkg, options) { // honored for "static", ignored for "head" and "body", sometimes // honored for CSS but ignored if we are concatenating. // - // sourceMap: Allowed only for "js". If present, a - // SourceMapGenerator. + // sourceMap: Allowed only for "js". If present, a string. // // sources: Only if 'sourceMap' present. A mapping from a relative // source path given in sourceMap (no leading '/') to: @@ -426,10 +425,7 @@ _.extend(Slice.prototype, { includePositionInErrors: options.lineForLine, linkerFileTransform: options.linkerFileTransform, bare: !!options.bare, - sourceMap: (options.sourceMapAsString - && sourcemap.SourceMapGenerator.fromSourceMap( - new sourcemap.SourceMapConsumer( - options.sourceMapAsString))), + sourceMap: options.sourceMap, sources: options.sources }); }, @@ -1917,10 +1913,8 @@ _.extend(Package.prototype, { }; if (resource.sourceMap) { rejectBadPath(resource.sourceMap); - var rawSourceMap = fs.readFileSync( + prelinkFile.sourceMap = fs.readFileSync( path.join(sliceBasePath, resource.sourceMap), 'utf8'); - prelinkFile.sourceMap = sourcemap.SourceMapGenerator.fromSourceMap( - new sourcemap.SourceMapConsumer(rawSourceMap)); if (resource.sources) { prelinkFile.sources = {}; _.each(resource.sources, function (x, pathForSourceMap) { @@ -2108,7 +2102,7 @@ _.extend(Package.prototype, { // Write the source map. resource.sourceMap = builder.writeToGeneratedFilename( path.join(sliceDir, file.servePath + '.map'), - { data: new Buffer(file.sourceMap.toString(), 'utf8') } + { data: new Buffer(file.sourceMap, 'utf8') } ); // Now write the sources themselves. From 96a1f43a3c4ee00ae4b5450a19dce6f57e449408 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Jul 2013 00:50:00 -0700 Subject: [PATCH 32/49] add a comment about line number comments on generated files --- tools/linker.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/linker.js b/tools/linker.js index d03a6d21b7..cb557023c1 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -425,7 +425,11 @@ _.extend(File.prototype, { buf += suffix; }); // The existing source map is valid because all we're doing is adding - // things to the end of lines, which doesn't affect the source map. + // things to the end of lines, which doesn't affect the source map. (If + // we wanted to be picky, we could add some explicitly non-mapped regions + // to the source map to cover the suffixes, which would make this + // equivalent to the "no source map coming in" case, but this doesn't seem + // that important.) chunks.push(sourcemap.SourceNode.fromStringWithSourceMap( self.source, new sourcemap.SourceMapConsumer(transformedSourceWithMap.sourceMap))); From 01a1bc8d6b50011a254daa64582f39ae9ce8f243 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Jul 2013 11:00:04 -0700 Subject: [PATCH 33/49] On server, parse source maps less often. --- scripts/generate-dev-bundle.sh | 2 +- tools/server/boot.js | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/scripts/generate-dev-bundle.sh b/scripts/generate-dev-bundle.sh index 1a62c2bb67..2ea7f6ce39 100755 --- a/scripts/generate-dev-bundle.sh +++ b/scripts/generate-dev-bundle.sh @@ -114,7 +114,7 @@ npm install source-map@0.1.24 # Fork of node-source-map-support which allows us to specify our own # retrieveSourceMap function. # XXX send a pull request -npm install https://github.com/meteor/node-source-map-support/tarball/3b90243e6b51a2ff8705acd74b62c09df05a6199 +npm install https://github.com/meteor/node-source-map-support/tarball/b23f7c5a509343b3951f5db2b5f790ad791bddc5 # uglify-js has a bug which drops 'undefined' in arrays: # https://github.com/mishoo/UglifyJS2/pull/97 diff --git a/tools/server/boot.js b/tools/server/boot.js index 587ab6e3f4..24febaddf4 100644 --- a/tools/server/boot.js +++ b/tools/server/boot.js @@ -37,19 +37,23 @@ if (!process.env.NODE_ENV) process.env.NODE_ENV = 'production'; // Map from load path to its source map. -var sourceMapsAsStrings = {}; +var parsedSourceMaps = {}; // Read all the source maps into memory once. _.each(serverJson.load, function (fileInfo) { if (fileInfo.sourceMap) { - sourceMapsAsStrings[fileInfo.path] = fs.readFileSync( + var rawSourceMap = fs.readFileSync( path.resolve(serverDir, fileInfo.sourceMap), 'utf8'); + // Parse the source map only once, not each time it's needed. Also remove + // the anti-XSSI header if it's there. + parsedSourceMaps[fileInfo.path] = JSON.parse( + rawSourceMap.replace(/^\)\]\}'/, '')); } }); var retrieveSourceMap = function (pathForSourceMap) { - if (_.has(sourceMapsAsStrings, pathForSourceMap)) - return {data: sourceMapsAsStrings[pathForSourceMap]}; + if (_.has(parsedSourceMaps, pathForSourceMap)) + return {map: parsedSourceMaps[pathForSourceMap]}; return null; }; From 269434a44b9c36dafed68d1ab36d6a0cbd64e97a Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Jul 2013 11:24:34 -0700 Subject: [PATCH 34/49] Inline source code in source maps instead of keeping another data structure. --- .../plugin/compile-coffeescript.js | 20 +++--- packages/webapp/webapp_server.js | 9 --- tools/bundler.js | 68 ++----------------- tools/linker.js | 53 ++++++--------- tools/packages.js | 35 +--------- tools/server/boot.js | 7 +- 6 files changed, 39 insertions(+), 153 deletions(-) diff --git a/packages/coffeescript/plugin/compile-coffeescript.js b/packages/coffeescript/plugin/compile-coffeescript.js index 2ac1d33f21..87bd15b9f2 100644 --- a/packages/coffeescript/plugin/compile-coffeescript.js +++ b/packages/coffeescript/plugin/compile-coffeescript.js @@ -119,9 +119,14 @@ var handler = function (compileStep) { bare: true, filename: compileStep.inputPath, literate: path.extname(compileStep.inputPath) === '.litcoffee', + // Return a source map. sourceMap: true, - generatedFile: "/" + outputFile, // Used in source map. - sourceFiles: [compileStep.pathForSourceMap] // Used in source map. + // Include the original source in the source map (sourcesContent field). + inline: true, + // This becomes the "file" field of the source map. + generatedFile: "/" + outputFile, + // This becomes the "sources" field of the source map. + sourceFiles: [compileStep.pathForSourceMap] }; try { @@ -135,14 +140,6 @@ var handler = function (compileStep) { ); } - // These are the sources that correspond to the input files in the source map. - var sources = {}; - sources[compileStep.pathForSourceMap] = { - source: new Buffer(source, 'utf8'), - package: compileStep.packageName, - sourcePath: compileStep.inputPath - }; - compileStep.addJavaScript({ path: outputFile, sourcePath: compileStep.inputPath, @@ -152,8 +149,7 @@ var handler = function (compileStep) { var stripped = stripExportedVars(source, exports); return addSharedHeader(stripped, sourceMap); }, - sourceMap: output.v3SourceMap, - sources: sources + sourceMap: output.v3SourceMap }); }; diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js index 66af9c24d1..297798b9ac 100644 --- a/packages/webapp/webapp_server.js +++ b/packages/webapp/webapp_server.js @@ -245,15 +245,6 @@ var runWebAppServer = function () { }; // Send the SourceMap header when the source file is served. staticFile.sourceMap = sourceMapRootUrl; - - // Now register all the sources from this source map to be served, under - // the source map's URL. - _.each(item.sources, function (x, pathForSourceMap) { - staticFiles[sourceMapRootUrl + pathForSourceMap] = { - path: x.source, - cacheable: true - }; - }); } staticFiles[url.parse(item.url).pathname] = staticFile; } diff --git a/tools/bundler.js b/tools/bundler.js index 9676eab042..23843b858e 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -71,7 +71,6 @@ // - size: size of file in bytes // - hash: sha1 hash of the file contents // - sourceMap: optional path to source map file (relative to program.json) -// - sources: same as in native format (see below) // Additionally there will be an entry with where equal to // "internal", path equal to page (above), and hash equal to the // sha1 of page (before replacements.) Currently this is used to @@ -108,15 +107,6 @@ // Assets.getText and Assets.getBinary are called from this file. // - sourceMap: if present, path of a file that contains a source // map for this file, relative to program.json -// - sources: if sourceMap present, a map from a a relative path in -// the source map (no leading slash) to information about the -// source file: -// - source: path of this source file if available, relative to -// program.json -// - package: name of the package from which this file came, if -// any (omit if file came from an app) -// - sourcePath: original relative path within the source tree of -// 'package' (or the app) of this source file // // /config.json: // @@ -247,8 +237,6 @@ var StaticDirectory = function (options) { // - sourcePath: path to file on disk that will provide our contents // - data: contents of the file as a Buffer // - sourceMap: if 'data' is given, can be given instead of sourcePath. a string -// - sources: if sourceMap is provided, map from relative path to object -// with keys 'package', 'sourcePath', 'source' // - cacheable var File = function (options) { var self = this; @@ -267,13 +255,6 @@ var File = function (options) { // information. Set with setSourceMap. self.sourceMap = null; - // If sourceMap is set, this is a map from a relative source file - // path in the source map (no leading '/') to an object with keys: - // - package: name of the source package, or null for the app - // - sourcePath: the source path, relative to the root of 'package' - // - source: full contents of the source file, as a Buffer - self.sources = null; - // Where this file is intended to reside within the target's // filesystem. self.targetPath = null; @@ -281,7 +262,7 @@ var File = function (options) { // The URL at which this file is intended to be served, relative to // the base URL at which the target is being served (ignored if this // file is not intended to be served over HTTP.) - self.url = null + self.url = null; // Is this file guaranteed to never change, so that we can let it be // cached forever? Only makes sense of self.url is set. @@ -396,15 +377,13 @@ _.extend(File.prototype, { }); }, - // Set a source map for this File. sourceMap is given as a string. See - // self.sources for the format of sources. - setSourceMap: function (sourceMap, sources) { + // Set a source map for this File. sourceMap is given as a string. + setSourceMap: function (sourceMap) { var self = this; if (typeof sourceMap !== "string") throw new Error("sourceMap must be given as a string"); self.sourceMap = sourceMap; - self.sources = sources || {}; } }); @@ -675,7 +654,7 @@ _.extend(Target.prototype, { } if (resource.type === "js" && resource.sourceMap) { - f.setSourceMap(resource.sourceMap, resource.sources); + f.setSourceMap(resource.sourceMap); } self[resource.type].push(f); @@ -901,17 +880,6 @@ _.extend(ClientTarget.prototype, { file.targetPath + '.map', { data: new Buffer(file.sourceMap, 'utf8') }); - - manifestItem.sources = {}; - _.each(file.sources, function (x, pathForSourceMap) { - manifestItem.sources[pathForSourceMap] = { - package: x.package, - sourcePath: x.sourcePath, - source: builder.writeToGeneratedFilename( - path.join('sources', pathForSourceMap), - { data: x.source }) - }; - }); } manifest.push(manifestItem); @@ -957,8 +925,6 @@ var JsImage = function () { // - nodeModulesDirectory: a NodeModulesDirectory indicating which // directory should be searched by Npm.require() // - sourceMap: if set, source map for this code, as a string - // - sources: map from relative path in source map to object with - // keys 'source' (a Buffer), 'package', 'sourcePath' // note: this can't be called `load` at it would shadow `load()` self.jsToLoad = []; @@ -1123,18 +1089,6 @@ _.extend(JsImage.prototype, { item.targetPath + '.map', { data: new Buffer(item.sourceMap, 'utf8') } ); - - // Now write the sources themselves. - loadItem.sources = {}; - _.each(item.sources, function (x, pathForSourceMap) { - loadItem.sources[pathForSourceMap] = { - package: x.package, - sourcePath: x.sourcePath, - source: builder.writeToGeneratedFilename( - path.join('sources', pathForSourceMap), - { data: x.source }) - }; - }); } load.push(loadItem); @@ -1207,17 +1161,6 @@ JsImage.readFromDisk = function (controlFilePath) { rejectBadPath(item.sourceMap); loadItem.sourceMap = fs.readFileSync( path.join(dir, item.sourceMap), 'utf8'); - if (item.sources) { - loadItem.sources = {}; - _.each(item.sources, function (x, pathForSourceMap) { - rejectBadPath(x.source); - loadItem.sources[pathForSourceMap] = { - package: x.package, - sourcePath: x.sourcePath, - source: fs.readFileSync(path.join(dir, x.source)) - }; - }); - } } ret.jsToLoad.push(loadItem); }); @@ -1248,8 +1191,7 @@ _.extend(JsImageTarget.prototype, { source: file.contents().toString('utf8'), nodeModulesDirectory: file.nodeModulesDirectory, staticDirectory: file.staticDirectory, - sourceMap: file.sourceMap, - sources: file.sources + sourceMap: file.sourceMap }); }); diff --git a/tools/linker.js b/tools/linker.js index cb557023c1..4b3a1bbf4e 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -93,9 +93,8 @@ _.extend(Module.prototype, { return _.isEmpty(globalReferences) ? undefined : globalReferences; }, - // Output is a list of objects with keys 'source', 'servePath', - // 'sourceMap', 'sources' (map from relative path in source map to - // 'package', 'sourcePath', 'source') + // Output is a list of objects with keys 'source', 'servePath', 'sourceMap', + // 'sourcePath', 'source') getPrelinkedFiles: function () { var self = this; @@ -115,14 +114,10 @@ _.extend(Module.prototype, { file: file.servePath }); // results has 'code' and 'map' attributes - var sources = {}; - file.addToSourcesSet(sources); - return { source: results.code, servePath: file.servePath, - sourceMap: results.map.toString(), - sources: sources + sourceMap: results.map.toString() }; }); } @@ -136,13 +131,11 @@ _.extend(Module.prototype, { var chunks = []; // Emit each file - var sources = {}; _.each(self.files, function (file) { if (!_.isEmpty(chunks)) chunks.push("\n\n\n\n\n\n"); chunks.push(file.getPrelinkedOutput({ sourceWidth: sourceWidth, exports: moduleExports })); - file.addToSourcesSet(sources); }); var node = new sourcemap.SourceNode(null, null, null, chunks); @@ -153,8 +146,7 @@ _.extend(Module.prototype, { return [{ source: results.code, servePath: self.combinedServePath, - sourceMap: results.map.toString(), - sources: sources + sourceMap: results.map.toString() }]; }, @@ -250,9 +242,6 @@ var File = function (inputFile, module) { // A source map (generated by something like CoffeeScript) for the input file. self.sourceMap = inputFile.sourceMap; - // Information about the source files in a source map. May have come with the - // source map, or we may be building it from scratch. - self.sourcesFromSourceMap = inputFile.sourceMap && inputFile.sources; // The Module containing this file. self.module = module; @@ -314,21 +303,6 @@ _.extend(File.prototype, { return "app/" + self.sourcePath; }, - // Add to a 'sources' set (a map from source map relative paths to - // info about each source file) - addToSourcesSet: function (sources) { - var self = this; - if (self.sourcesFromSourceMap) { - _.extend(sources, self.sourcesFromSourceMap); - } else { - sources[self._pathForSourceMap()] = { - package: self.module.name, - sourcePath: self.sourcePath, - source: new Buffer(self.source, 'utf8') // XXX encoding - }; - } - }, - // Options: // - preserveLineNumbers: if true, decorate minimally so that line // numbers don't change between input and output. In this case, @@ -354,6 +328,8 @@ _.extend(File.prototype, { } else { mapNode = new sourcemap.SourceNode(1, 0, self._pathForSourceMap(), self.source); + // Provide the original content as well. + mapNode.setSourceContent(self._pathForSourceMap(), self.source); } return new sourcemap.SourceNode(null, null, null, [ @@ -394,6 +370,9 @@ _.extend(File.prototype, { var transformedSourceWithMap = self.linkerFileTransform( self.source, options.exports, self.sourceMap); + // Update self.source, because that's what computeGlobalReferences will look + // at. + var originalSource = self.source; self.source = transformedSourceWithMap.source; // Code, with line numbers @@ -451,6 +430,14 @@ _.extend(File.prototype, { var node = new sourcemap.SourceNode(null, null, null, chunks); + // If we're working directly from the original source here (and not from the + // output of a transformation that had a source map), include the original + // source in the source map. (If we are working on generated code, the + // source map we received should have already contained the original + // source.) + if (!transformedSourceWithMap.sourceMap) + node.setSourceContent(self._pathForSourceMap(), originalSource); + return node; }, @@ -571,8 +558,7 @@ _.extend(File.prototype, { // Output is an object with keys: // - files: is an array of output files in the same format as inputFiles // - EXCEPT THAT, for now, sourcePath is omitted and is replaced with -// sourceMap (a string) and sources (map to keys 'package', -// 'sourcePath', 'path') similar to self.resources in a Slice (XXX) +// sourceMap (a string) (XXX) // - exports: the exports, as a list of string ('Foo', 'Thing.Stuff', etc) var prelink = function (options) { if (options.noExports && options.forceExport && @@ -656,8 +642,7 @@ var link = function (options) { ret.push({ source: results.code, servePath: file.servePath, - sourceMap: results.map.toString(), - sources: file.sources + sourceMap: results.map.toString() }); } else { ret.push({ diff --git a/tools/packages.js b/tools/packages.js index 59b7b922d6..1c670ee2e7 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -183,12 +183,6 @@ var Slice = function (pkg, options) { // // sourceMap: Allowed only for "js". If present, a string. // - // sources: Only if 'sourceMap' present. A mapping from a relative - // source path given in sourceMap (no leading '/') to: - // - package: package name, or null for app - // - sourcePath: original relative path within 'package' or app - // - source: full contents of the original source file, as a Buffer - // // Set only when isBuilt is true. self.resources = null; @@ -425,8 +419,7 @@ _.extend(Slice.prototype, { includePositionInErrors: options.lineForLine, linkerFileTransform: options.linkerFileTransform, bare: !!options.bare, - sourceMap: options.sourceMap, - sources: options.sources + sourceMap: options.sourceMap }); }, addAsset: function (options) { @@ -571,8 +564,7 @@ _.extend(Slice.prototype, { data: new Buffer(file.source, 'utf8'), // XXX encoding servePath: file.servePath, staticDirectory: self.staticDirectory, - sourceMap: file.sourceMap, - sources: file.sources + sourceMap: file.sourceMap }; }); @@ -1915,17 +1907,6 @@ _.extend(Package.prototype, { rejectBadPath(resource.sourceMap); prelinkFile.sourceMap = fs.readFileSync( path.join(sliceBasePath, resource.sourceMap), 'utf8'); - if (resource.sources) { - prelinkFile.sources = {}; - _.each(resource.sources, function (x, pathForSourceMap) { - rejectBadPath(x.source); - prelinkFile.sources[pathForSourceMap] = { - package: x.package, - sourcePath: x.sourcePath, - source: fs.readFileSync(path.join(sliceBasePath, x.source)) - }; - }); - } } slice.prelinkFiles.push(prelinkFile); } else if (_.contains(["head", "body", "css", "js", "static"], @@ -2104,18 +2085,6 @@ _.extend(Package.prototype, { path.join(sliceDir, file.servePath + '.map'), { data: new Buffer(file.sourceMap, 'utf8') } ); - - // Now write the sources themselves. - resource.sources = {}; - _.each(file.sources, function (x, pathForSourceMap) { - resource.sources[pathForSourceMap] = { - package: x.package, - sourcePath: x.sourcePath, - source: builder.writeToGeneratedFilename( - path.join(sliceDir, 'sources', x.sourcePath), - { data: x.source }) - }; - }); } sliceJson.resources.push(resource); diff --git a/tools/server/boot.js b/tools/server/boot.js index 24febaddf4..b4f0c8011f 100644 --- a/tools/server/boot.js +++ b/tools/server/boot.js @@ -46,8 +46,11 @@ _.each(serverJson.load, function (fileInfo) { path.resolve(serverDir, fileInfo.sourceMap), 'utf8'); // Parse the source map only once, not each time it's needed. Also remove // the anti-XSSI header if it's there. - parsedSourceMaps[fileInfo.path] = JSON.parse( - rawSourceMap.replace(/^\)\]\}'/, '')); + var parsedSourceMap = JSON.parse(rawSourceMap.replace(/^\)\]\}'/, '')); + // source-map-support doesn't ever look at the sourcesContent field, so + // there's no point in keeping it in memory. + delete parsedSourceMap.sourcesContent; + parsedSourceMaps[fileInfo.path] = parsedSourceMap; } }); From be0611d262feb3bdaa74bff3d243f247934a5585 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Jul 2013 12:00:33 -0700 Subject: [PATCH 35/49] One attempt at making source map URLs cleaner. You get the source map for /foo.js by asking for /foo.js?sourcemap=y. Source files then implicitly show up next to them in the tree. --- packages/webapp/webapp_server.js | 47 ++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js index 297798b9ac..6453c9536d 100644 --- a/packages/webapp/webapp_server.js +++ b/packages/webapp/webapp_server.js @@ -215,9 +215,8 @@ var runWebAppServer = function () { next(); } }); - // Parse the query string into res.query. Only oauth_server cares about this, - // but it's overkill to have that package depend on its own copy of connect - // just for this simple processing. + // Parse the query string into res.query. Used to detect sourcemap=1 and by + // oauth_server. app.use(connect.query()); // Auto-compress any json, javascript, or text. @@ -237,14 +236,18 @@ var runWebAppServer = function () { // so that source files referenced from the source map with relative URLs // are resolved under it. if (item.sourceMap) { - var sourceMapRootUrl = "/_sources/" + sha1(item.url) + "/"; - // Register the source map itself to be served. - staticFiles[sourceMapRootUrl] = { - path: item.sourceMap, - cacheable: true - }; + var sourceMapUrl = item.url; + var argument = "sourcemap=y"; + if (sourceMapUrl.indexOf('?') === -1) + sourceMapUrl += '?' + argument; + else + sourceMapUrl += '&' + argument; + // Send the SourceMap header when the source file is served. - staticFile.sourceMap = sourceMapRootUrl; + staticFile.urlToSourceMap = sourceMapUrl; + + // Register the source map itself to be served. + staticFile.sourceMapFile = item.sourceMap; } staticFiles[url.parse(item.url).pathname] = staticFile; } @@ -276,6 +279,18 @@ var runWebAppServer = function () { var info = staticFiles[pathname]; + var fileToServe, urlToSourceMap; + if (req.query.sourcemap === 'y') { + // This is a request for a sourcemap. + fileToServe = info.sourceMapFile; + // Source maps don't have source maps. + urlToSourceMap = null; + } else { + // This is not a request for a sourcemap. + fileToServe = info.path; + urlToSourceMap = info.urlToSourceMap; + } + // Cacheable files are files that should never change. Typically // named by their hash (eg meteor bundled js and css files). // We cache them ~forever (1yr). @@ -287,12 +302,16 @@ var runWebAppServer = function () { // to include a query parameter to bust caches. That way we can both get // good caching behavior and allow users to change assets without delay. // https://github.com/meteor/meteor/issues/773 + // + // Because a source map's URL is constructed from the URL of the file it is + // the source map of, we assume that a source map is cacheable iff the file + // is cacheable. var maxAge = info.cacheable ? 1000 * 60 * 60 * 24 * 365 : 1000 * 60 * 60 * 24; // Tell the client where to find the source map for this file. - if (info.sourceMap) { + if (urlToSourceMap) { // This should just be SourceMap, but slightly more versions of Chrome // support the older X-SourceMap. // @@ -321,10 +340,10 @@ var runWebAppServer = function () { // - not using the send module (or hacking it to allow a footer) // None of these alternatives are great, so for now we just hope that // FF will implement one of the headers soon. - res.setHeader('X-SourceMap', info.sourceMap); + res.setHeader('X-SourceMap', urlToSourceMap); } - send(req, path.join(clientDir, info.path)) + send(req, path.join(clientDir, fileToServe)) .maxage(maxAge) .hidden(true) // if we specified a dotfile in the manifest, serve it .on('error', function (err) { @@ -333,7 +352,7 @@ var runWebAppServer = function () { res.end(); }) .on('directory', function () { - Log.error("Unexpected directory " + info.path); + Log.error("Unexpected directory " + fileToServe); res.writeHead(500); res.end(); }) From 56f663ea67897f718038c506d5faaa575b38042a Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Jul 2013 13:30:38 -0700 Subject: [PATCH 36/49] Add anti-XSSI header to source maps in client programs. This is done at bundle time when constructing the manifest; it does not affect source maps in unipackages, etc. --- tools/bundler.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tools/bundler.js b/tools/bundler.js index 23843b858e..eb2edc9fe1 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -876,10 +876,15 @@ _.extend(ClientTarget.prototype, { }; if (file.sourceMap) { + // Add anti-XSSI header to this file which will be served over + // HTTP. Note that the Mozilla and WebKit implementations differ as to + // what they strip: Mozilla looks for the four punctuation characters + // but doesn't care about the newline; WebKit only looks for the first + // three characters (not the single quote) and then strips everything up + // to a newline. + var mapData = new Buffer(")]}'\n" + file.sourceMap, 'utf8'); manifestItem.sourceMap = builder.writeToGeneratedFilename( - file.targetPath + '.map', { - data: new Buffer(file.sourceMap, 'utf8') - }); + file.targetPath + '.map', {data: mapData}); } manifest.push(manifestItem); From 85ae17ae0f3a72283f047aa841ebb1d329848331 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Jul 2013 14:02:42 -0700 Subject: [PATCH 37/49] Add instructions on enabling source maps to linked browser code. --- tools/linker.js | 76 +++++++++++++++++++++++++++++++++-------------- tools/packages.js | 1 + 2 files changed, 54 insertions(+), 23 deletions(-) diff --git a/tools/linker.js b/tools/linker.js index 4b3a1bbf4e..4cf781ac19 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -344,29 +344,23 @@ _.extend(File.prototype, { // Pretty version var chunks = []; - var header = ""; // Prologue if (! self.bare) - header += "(function () {\n\n"; + chunks.push("(function () {\n\n"); // Banner + var bannerLines = [self.servePath.slice(1)]; + if (self.bare) { + bannerLines.push( + "This file is in bare mode and is not in its own closure."); + } var width = options.sourceWidth || 70; var bannerWidth = width + 3; - var divider = new Array(bannerWidth + 1).join('/') + "\n"; - var spacer = "// " + new Array(bannerWidth - 6 + 1).join(' ') + " //\n"; - var padding = new Array(bannerWidth + 1).join(' '); + var padding = bannerPadding(bannerWidth); + chunks.push(banner(bannerLines, bannerWidth)); var blankLine = new Array(width + 1).join(' ') + " //\n"; - header += divider + spacer; - header += "// " + - (self.servePath.slice(1) + padding).slice(0, bannerWidth - 6) + " //\n"; - if (self.bare) { - var bareText = "This file is in bare mode and is not in its own closure."; - header += "// " + - (bareText + padding).slice(0, bannerWidth - 6) + " //\n"; - } - header += spacer + divider + blankLine; - chunks.push(header); + chunks.push(blankLine); var transformedSourceWithMap = self.linkerFileTransform( self.source, options.exports, self.sourceMap); @@ -426,7 +420,7 @@ _.extend(File.prototype, { // Footer if (! self.bare) - chunks.push(divider + "\n}).call(this);\n"); + chunks.push(dividerLine(bannerWidth) + "\n}).call(this);\n"); var node = new sourcemap.SourceNode(null, null, null, chunks); @@ -501,6 +495,32 @@ _.extend(File.prototype, { } }); +// Given a list of lines (not newline-terminated), returns a string placing them +// in a pretty banner of width bannerWidth. All lines must have length at most +// (bannerWidth - 6); if bannerWidth is not provided, the smallest width that +// fits is used. +var banner = function (lines, bannerWidth) { + if (!bannerWidth) + bannerWidth = 6 + _.max(lines, function (x) { return x.length; }).length; + + var divider = dividerLine(bannerWidth); + var spacer = "// " + new Array(bannerWidth - 6 + 1).join(' ') + " //\n"; + var padding = bannerPadding(bannerWidth); + + var buf = divider + spacer; + _.each(lines, function (line) { + buf += "// " + (line + padding).slice(0, bannerWidth - 6) + " //\n"; + }); + buf += spacer + divider; + return buf; +}; +var dividerLine = function (bannerWidth) { + return new Array(bannerWidth + 1).join('/') + "\n"; +}; +var bannerPadding = function (bannerWidth) { + return new Array(bannerWidth + 1).join(' '); +}; + /////////////////////////////////////////////////////////////////////////////// // Top-level entry point /////////////////////////////////////////////////////////////////////////////// @@ -591,6 +611,14 @@ var prelink = function (options) { }; }; +var SOURCE_MAP_INSTRUCTIONS_COMMENT = banner([ + "This file was compiled by the Meteor linker. You can view the original", + "source in your browser if your browser supports source maps.", + "", + "If you are using Chrome, open the Developer Tools and click the gear", + "icon in its lower right corner. In the General Settings panel, turn", + "on 'Enable source maps'." +]); // Finish the linking. // @@ -621,7 +649,8 @@ var link = function (options) { var header = getHeader({ imports: options.imports, - packageScopeVariables: options.packageScopeVariables}); + packageScopeVariables: options.packageScopeVariables + }); var footer = getFooter({ exports: options.exports, name: options.name @@ -630,12 +659,13 @@ var link = function (options) { var ret = []; _.each(options.prelinkFiles, function (file) { if (file.sourceMap) { - var node = new sourcemap.SourceNode(null, null, null, [ - header, - sourcemap.SourceNode.fromStringWithSourceMap( - file.source, new sourcemap.SourceMapConsumer(file.sourceMap)), - footer - ]); + var chunks = [header]; + if (options.includeSourceMapInstructions) + chunks.push("\n" + SOURCE_MAP_INSTRUCTIONS_COMMENT + "\n\n"); + chunks.push(sourcemap.SourceNode.fromStringWithSourceMap( + file.source, new sourcemap.SourceMapConsumer(file.sourceMap))); + chunks.push(footer); + var node = new sourcemap.SourceNode(null, null, null, chunks); var results = node.toStringWithSourceMap({ file: file.servePath }); diff --git a/tools/packages.js b/tools/packages.js index 1c670ee2e7..2f13f063fb 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -554,6 +554,7 @@ _.extend(Slice.prototype, { prelinkFiles: self.prelinkFiles, exports: self.exports, packageScopeVariables: self.packageScopeVariables, + includeSourceMapInstructions: archinfo.matches(self.arch, "browser"), name: self.pkg.name || null }); From faf49921bed26de75c709a1682d3e1dc75d02b17 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Jul 2013 16:23:05 -0700 Subject: [PATCH 38/49] replace barely-used lineForLine/includePositionInErrors with use of source maps. Note that this is only triggered when coffeescript manages to output invalid JS, which should be unlikely. This does remove the feature where lines and columns were suppressed for parse errors in the output of template compilation. but (a) that shouldn't happen, and (b) we'll fix this by implementing source maps for spacebars. --- .../plugin/compile-coffeescript.js | 1 - .../templating/plugin/compile-templates.js | 7 +++-- tools/linker.js | 28 ++++++++++++------- tools/packages.js | 10 ++----- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/packages/coffeescript/plugin/compile-coffeescript.js b/packages/coffeescript/plugin/compile-coffeescript.js index 87bd15b9f2..b2e2ea4df9 100644 --- a/packages/coffeescript/plugin/compile-coffeescript.js +++ b/packages/coffeescript/plugin/compile-coffeescript.js @@ -144,7 +144,6 @@ var handler = function (compileStep) { path: outputFile, sourcePath: compileStep.inputPath, data: output.js, - lineForLine: false, linkerFileTransform: function (source, exports, sourceMap) { var stripped = stripExportedVars(source, exports); return addSharedHeader(stripped, sourceMap); diff --git a/packages/templating/plugin/compile-templates.js b/packages/templating/plugin/compile-templates.js index 1effe1a718..7f22bf6b83 100644 --- a/packages/templating/plugin/compile-templates.js +++ b/packages/templating/plugin/compile-templates.js @@ -42,11 +42,12 @@ Plugin.registerSourceHandler("html", function (compileStep) { var ext = path.extname(compileStep.inputPath); var basename = path.basename(compileStep.inputPath, ext); + // XXX generate a source map + compileStep.addJavaScript({ path: path.join(path_part, "template." + basename + ".js"), sourcePath: compileStep.inputPath, - data: results.js, - lineForLine: false + data: results.js }); } -}); \ No newline at end of file +}); diff --git a/tools/linker.js b/tools/linker.js index 4cf781ac19..42ecad44f4 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -227,9 +227,6 @@ var File = function (inputFile, module) { // package or app.) Used for source maps, error messages.. self.sourcePath = inputFile.sourcePath; - // should line and column be included in errors? - self.includePositionInErrors = inputFile.includePositionInErrors; - // A function which transforms the source code once all exports are // known. (eg, for CoffeeScript.) self.linkerFileTransform = @@ -277,12 +274,25 @@ _.extend(File.prototype, { } catch (e) { if (!e.$ParseError) throw e; - buildmessage.error(e.description, { + + var errorOptions = { file: self.sourcePath, - line: self.includePositionInErrors ? e.lineNumber : null, - column: self.includePositionInErrors ? e.column : null, + line: e.lineNumber, + column: e.column, downcase: true - }); + }; + if (self.sourceMap) { + var parsed = new sourcemap.SourceMapConsumer(self.sourceMap); + var original = parsed.originalPositionFor( + {line: e.lineNumber, column: e.column - 1}); + if (original.source) { + errorOptions.file = original.source; + errorOptions.line = original.line; + errorOptions.column = original.column + 1; + } + } + + buildmessage.error(e.description, errorOptions); // Recover by pretending that this file is empty (which // includes replacing its source code with '' in the output) @@ -545,9 +555,7 @@ var bannerPadding = function (bannerWidth) { // bundle, not in error messages, but it's still nice to make it // look good) // - sourcePath: path to use in error messages -// - includePositionInErrors: true to include line and column -// information in errors. set to false if, eg, this is the output -// of coffeescript. (XXX replace with real sourcemaps) +// - sourceMap: an optional source map (as string) for the input file // - linkerFileTransform: if given, this function will be called // when the module is being linked with the source of the file // and an array of the exports of the module; the file's source will diff --git a/tools/packages.js b/tools/packages.js index 2f13f063fb..5266e4432b 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -294,7 +294,7 @@ _.extend(Slice.prototype, { // effect, such as minification.) // - addJavaScript({ path: "my/program.js", data: "my code", // sourcePath: "src/my/program.js", - // lineForLine: true, bare: true}) + // bare: true }) // Add JavaScript code, which will be namespaced into this // package's environment (eg, it will see only the exports of // this package's imports), and which will be subject to @@ -303,11 +303,7 @@ _.extend(Slice.prototype, { // that will be used in any error messages generated (eg, // "foo.js:4:1: syntax error"). It must be present and should // be relative to the project root. Typically 'inputPath' will - // do handsomely. Set the misleadingly named lineForLine - // option to true if line X, column Y in the input corresponds - // to line X, column Y in the output. This will enable line - // and column reporting in error messages. (XXX replace this - // with source maps) "bare" means to not wrap the file in + // do handsomely. "bare" means to not wrap the file in // a closure, so that its vars are shared with other files // in the module. // - addAsset({ path: "my/image.png", data: Buffer }) @@ -416,7 +412,6 @@ _.extend(Slice.prototype, { source: options.data, sourcePath: options.sourcePath, servePath: path.join(self.pkg.serveRoot, options.path), - includePositionInErrors: options.lineForLine, linkerFileTransform: options.linkerFileTransform, bare: !!options.bare, sourceMap: options.sourceMap @@ -615,7 +610,6 @@ _.extend(Slice.prototype, { data: compileStep.read().toString('utf8'), path: compileStep.inputPath, sourcePath: compileStep.inputPath, - lineForLine: true, // XXX eventually get rid of backward-compatibility "raw" name bare: compileStep.fileOptions.bare || compileStep.fileOptions.raw }); From 89e2fb1df1e4972aa93174a3be501bf6b0c21272 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Jul 2013 16:39:20 -0700 Subject: [PATCH 39/49] Fix static file serving tests now that we've changed how static files are served. Some things that used to serve 403s now serve app HTML. Some paths with ..'s that used to resolve to actual app resources now don't. --- packages/http/httpcall_tests.js | 40 ++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/packages/http/httpcall_tests.js b/packages/http/httpcall_tests.js index 115806ab05..fddf48b032 100644 --- a/packages/http/httpcall_tests.js +++ b/packages/http/httpcall_tests.js @@ -441,33 +441,37 @@ if (Meteor.isServer) { })); }; - // no such file - do_test("/nosuchfile", 200, /DOCTYPE/); - do_test("/../nosuchfile", 403); - do_test("/%2e%2e/nosuchfile", 403); - do_test("/%2E%2E/nosuchfile", 403); - do_test("/%2d%2d/nosuchfile", 200, /DOCTYPE/); - // existing static file - var succeeds = [ - "/packages/http/test_static.serveme", + do_test("/packages/http/test_static.serveme", 200, /static file serving/); + + // no such file, so return the default app HTML. + var getsAppHtml = [ + // This file doesn't exist. + "/nosuchfile", + + // Our static file serving doesn't process .. or its encoded version, so + // any of these return the app HTML. + "/../nosuchfile", + "/%2e%2e/nosuchfile", + "/%2E%2E/nosuchfile", + "/%2d%2d/nosuchfile", "/packages/http/../http/test_static.serveme", "/packages/http/%2e%2e/http/test_static.serveme", "/packages/http/%2E%2E/http/test_static.serveme", "/packages/http/../../packages/http/test_static.serveme", "/packages/http/%2e%2e/%2e%2e/packages/http/test_static.serveme", "/packages/http/%2E%2E/%2E%2E/packages/http/test_static.serveme", + + // ... and they *definitely* shouldn't be able to escape the app bundle. + "/packages/http/../../../../../../packages/http/test_static.serveme", + "/../../../../../../../../../../../bin/ls", + "/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/bin/ls", + "/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/bin/ls" ]; - _.each(succeeds, function (path) { - do_test(path, 200, /static file serving/); + + _.each(getsAppHtml, function (x) { + do_test(x, 200, /Tests<\/title/); }); - do_test("/packages/http/../../../../../../packages/http/test_static.serveme", 403); - - // file outside of our app - do_test("/../../../../../../../../../../../bin/ls", 403); - do_test("/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/bin/ls", 403); - do_test("/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/%2E%2E/bin/ls", 403); - } ]); } From a8ccc8786d84e9e6fbab48644e3e05e454133c00 Mon Sep 17 00:00:00 2001 From: David Glasser <glasser@meteor.com> Date: Wed, 10 Jul 2013 16:40:59 -0700 Subject: [PATCH 40/49] Add link to discussion of sourcemap XSSI header. --- tools/bundler.js | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/bundler.js b/tools/bundler.js index eb2edc9fe1..44cb033226 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -882,6 +882,7 @@ _.extend(ClientTarget.prototype, { // but doesn't care about the newline; WebKit only looks for the first // three characters (not the single quote) and then strips everything up // to a newline. + // https://groups.google.com/forum/#!topic/mozilla.dev.js-sourcemap/3QBr4FBng5g var mapData = new Buffer(")]}'\n" + file.sourceMap, 'utf8'); manifestItem.sourceMap = builder.writeToGeneratedFilename( file.targetPath + '.map', {data: mapData}); From 81e456ae8171984df958a723ecc65657c2e4b42d Mon Sep 17 00:00:00 2001 From: David Glasser <glasser@meteor.com> Date: Wed, 10 Jul 2013 17:24:56 -0700 Subject: [PATCH 41/49] Improve log lines file naming on server. We avoid using Error.prepareStackTrace (which the node source map support also uses) and do some hacky regexp parsing instead. This way, on the server, we get the filename/line numbers after source map processing. On the client we continue to get the compiled version... I guess because source maps are implemented in the developer tools, not directly into the Error object. (Probably should have gotten parseStack from tools/buildmessage.js instead.) unbreaks _getCallerDetails tests. --- packages/logging/logging.js | 50 +++++++++++++++++--------------- packages/logging/logging_test.js | 7 ++--- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/packages/logging/logging.js b/packages/logging/logging.js index 227e99c782..db33a5ca07 100644 --- a/packages/logging/logging.js +++ b/packages/logging/logging.js @@ -75,11 +75,11 @@ var logInBrowser = function (obj) { // @returns {Object: { line: Number, file: String }} Log._getCallerDetails = function () { var getStack = function () { - var orig = Error.prepareStackTrace; - Error.prepareStackTrace = function(_, stack){ return stack; }; + // We do NOT use Error.prepareStackTrace here (a V8 extension that gets us a + // pre-parsed stack) since it's impossible to compose it with the use of + // Error.prepareStackTrace used on the server for source maps. var err = new Error; var stack = err.stack; - Error.prepareStackTrace = orig; return stack; }; @@ -87,33 +87,37 @@ Log._getCallerDetails = function () { if (!stack) return {}; - var isV8 = false; - var lines = stack; - // check for V8 specifics - if (_.isArray(stack)) - isV8 = true; - else - lines = stack.split('\n'); - var index = 1; - var line = lines[index]; + var lines = stack.split('\n'); - // looking for the first line outside the logging package - while ((isV8 ? line.getFileName() || '' : line) - .indexOf('/packages/logging.js') !== -1) - line = lines[++index]; + // looking for the first line outside the logging package (or an + // eval if we find that first) + var line; + for (var i = 1; i < lines.length; ++i) { + line = lines[i]; + if (line.match(/^\s*at eval \(eval/)) { + return {file: "eval"}; + } + + // XXX probably wants to be / or .js in case no source maps + if (!line.match(/packages\/logging(?:\/|(?:\.tests)?\.js)/)) + break; + } var details = {}; - // The format for FF is functionName@filePath:lineNumber - // For V8 call built-in function - details.line = isV8 ? line.getLineNumber() : line.split(':').slice(-1)[0]; + // The format for FF is 'functionName@filePath:lineNumber' + // The format for V8 is 'functionName (packages/logging/logging.js:81)' or + // 'packages/logging/logging.js:81' + var match = /(?:[@(]| at )([^(]+?):([0-9:]+)(?:\)|$)/.exec(line); + if (!match) + return details; + // in case the matched block here is line:column + details.line = match[2].split(':')[0]; // Possible format: https://foo.bar.com/scripts/file.js?random=foobar - // For FF we parse the line, for V8 we call built-in function // XXX: if you can write the following in better way, please do it - details.file = isV8 ? line.getFileName() || (line.isEval() ? 'eval' : '') - : line.split('@')[1].split(':').slice(0, -1).join(':'); - details.file = details.file.split('/').slice(-1)[0].split('?')[0]; + // XXX: what about evals? + details.file = match[1].split('/').slice(-1)[0].split('?')[0]; return details; }; diff --git a/packages/logging/logging_test.js b/packages/logging/logging_test.js index 16cc748f7e..4305abf7e0 100644 --- a/packages/logging/logging_test.js +++ b/packages/logging/logging_test.js @@ -4,14 +4,11 @@ Tinytest.add("logging - _getCallerDetails", function (test) { // in Chrome and Firefox, other browsers don't give us an ability to get // stacktrace. if ((new Error).stack) { - //test.equal(details, { file: 'logging_test.js', line: 2 }); - // XXX: When we have source maps, we should uncomment the test above and - // remove this one - test.isTrue(details.file === 'logging.tests.js'); + test.equal(details.file, 'tinytest.js'); // evaled statements shouldn't crash var code = "Log._getCallerDetails().file"; - test.matches(eval(code), /^eval|logging.tests.js$/); + test.matches(eval(code), /^eval|tinytest.js$/); } }); From 3fb6a8c602fdbeff9c85aa1b8b13617d6e222aff Mon Sep 17 00:00:00 2001 From: David Glasser <glasser@meteor.com> Date: Thu, 11 Jul 2013 11:24:12 -0700 Subject: [PATCH 42/49] add some comments to files.runJavaScript --- tools/files.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tools/files.js b/tools/files.js index 93f5ef26e4..51801be2f6 100644 --- a/tools/files.js +++ b/tools/files.js @@ -642,6 +642,15 @@ _.extend(exports, { // another copy of node and feed it the offending code on // stdin. It should give us the error on stderr. + // Why do we think it has to be a parse error? After all, this is + // *run*InThisContext so it could be a thrown exception. But the only code + // we are running is the function definition, which should not be able to + // throw... unless "code" is imbalanced (ie, has a '})' in it), in which + // case "code" should basically be considered as a parse error + // anyway. (The error message in the latter case may be poor, though.) + + // XXX XXX don't run the code until after this block + var Future = require('fibers/future'); var future = new Future; From 177a5684c9f9336acd178373b857420deb1c1c3f Mon Sep 17 00:00:00 2001 From: David Glasser <glasser@meteor.com> Date: Thu, 11 Jul 2013 14:17:17 -0700 Subject: [PATCH 43/49] checkpoint for using source maps in jsimage.load syntax error parsing is broken, will fix --- .../plugin/compile-coffeescript.js | 6 +- .../.npm/package/npm-shrinkwrap.json | 2 +- scripts/generate-dev-bundle.sh | 10 ++- tools/buildmessage.js | 15 ++-- tools/bundler.js | 9 +- tools/files.js | 84 +++++++++++++++---- tools/linker.js | 70 +++++++++------- tools/packages.js | 6 +- 8 files changed, 139 insertions(+), 63 deletions(-) diff --git a/packages/coffeescript/plugin/compile-coffeescript.js b/packages/coffeescript/plugin/compile-coffeescript.js index b2e2ea4df9..988eb53188 100644 --- a/packages/coffeescript/plugin/compile-coffeescript.js +++ b/packages/coffeescript/plugin/compile-coffeescript.js @@ -144,9 +144,9 @@ var handler = function (compileStep) { path: outputFile, sourcePath: compileStep.inputPath, data: output.js, - linkerFileTransform: function (source, exports, sourceMap) { - var stripped = stripExportedVars(source, exports); - return addSharedHeader(stripped, sourceMap); + linkerFileTransform: function (sourceWithMap, exports) { + var stripped = stripExportedVars(sourceWithMap.source, exports); + return addSharedHeader(stripped, sourceWithMap.sourceMap); }, sourceMap: output.v3SourceMap }); diff --git a/packages/mongo-livedata/.npm/package/npm-shrinkwrap.json b/packages/mongo-livedata/.npm/package/npm-shrinkwrap.json index 03ac6db249..414c6ec42f 100644 --- a/packages/mongo-livedata/.npm/package/npm-shrinkwrap.json +++ b/packages/mongo-livedata/.npm/package/npm-shrinkwrap.json @@ -7,7 +7,7 @@ "version": "0.1.9" }, "kerberos": { - "version": "0.0.2" + "version": "0.0.3" } } } diff --git a/scripts/generate-dev-bundle.sh b/scripts/generate-dev-bundle.sh index 2ea7f6ce39..da49a6d517 100755 --- a/scripts/generate-dev-bundle.sh +++ b/scripts/generate-dev-bundle.sh @@ -109,12 +109,16 @@ npm install tar@0.1.14 npm install kexec@0.1.1 npm install shell-quote@0.0.1 npm install byline@2.0.3 -npm install source-map@0.1.24 + +# Fork of source-map which fixes one function with empty maps. +# https://github.com/mozilla/source-map/pull/70 +# See also below, where we get it into source-map-support. +npm install https://github.com/meteor/source-map/tarball/4a52398901fdb4b55b06ef4dd8b69f8256072b09 # Fork of node-source-map-support which allows us to specify our own -# retrieveSourceMap function. +# retrieveSourceMap function, and uses the above version of source-map. # XXX send a pull request -npm install https://github.com/meteor/node-source-map-support/tarball/b23f7c5a509343b3951f5db2b5f790ad791bddc5 +npm install https://github.com/meteor/node-source-map-support/tarball/d048eaa765bf743ddaad64716647f8760e2b8507 # uglify-js has a bug which drops 'undefined' in arrays: # https://github.com/mishoo/UglifyJS2/pull/97 diff --git a/tools/buildmessage.js b/tools/buildmessage.js index b8860e6928..650d87787c 100644 --- a/tools/buildmessage.js +++ b/tools/buildmessage.js @@ -349,8 +349,7 @@ var error = function (message, options) { // Record an exception. The message as well as any file and line // information be read directly out of the exception. If not in a job, -// throws the exception instead. The first character of the error's -// message is downcased. Also capture the user portion of the stack. +// throws the exception instead. Also capture the user portion of the stack. // // There is special handling for files.FancySyntaxError exceptions. We // will grab the file and location information where the syntax error @@ -358,23 +357,25 @@ var error = function (message, options) { // thrown. var exception = function (error) { if (! currentJob) - throw new Error("Error: " + error.message); + throw error; - var message = error.message.slice(0,1).toLowerCase() + error.message.slice(1); + var message = error.message; if (error instanceof files.FancySyntaxError) { + // No stack, because FancySyntaxError isn't a real Error and has no stack + // property! currentJob.addMessage({ message: message, - stack: parseStack(error), file: error.file, line: error.line, column: error.column }); } else { - var locus = parseStack(error)[0]; + var stack = parseStack(error); + var locus = stack[0]; currentJob.addMessage({ message: message, - stack: parseStack(error), + stack: stack, func: locus.func, file: locus.file, line: locus.line, diff --git a/tools/bundler.js b/tools/bundler.js index 44cb033226..096999e991 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -1036,9 +1036,12 @@ _.extend(JsImage.prototype, { try { // XXX XXX Get the actual source file path -- item.targetPath // is not actually correct (it's the path in the bundle rather - // than in the source tree.) Moreover, we need to do source - // mapping. - files.runJavaScript(item.source, item.targetPath, env); + // than in the source tree.) + files.runJavaScript(item.source.toString('utf8'), { + filename: item.targetPath, + symbols: env, + sourceMap: item.sourceMap + }); } catch (e) { buildmessage.exception(e); // Recover by skipping the rest of the load diff --git a/tools/files.js b/tools/files.js index 51801be2f6..6787ff5e85 100644 --- a/tools/files.js +++ b/tools/files.js @@ -10,10 +10,31 @@ var os = require('os'); var util = require('util'); var _ = require('underscore'); var Future = require('fibers/future'); +var sourcemap = require('source-map'); +var sourcemap_support = require('source-map-support'); var cleanup = require('./cleanup.js'); var buildmessage = require('./buildmessage.js'); +var parsedSourceMaps = {}; +var nextStackFilenameCounter = 1; +var retrieveSourceMap = function (pathForSourceMap) { + if (_.has(parsedSourceMaps, pathForSourceMap)) + return {map: parsedSourceMaps[pathForSourceMap]}; + return null; +}; + +sourcemap_support.install({ + // Use the source maps specified to runJavaScript instead of parsing source + // code for them. + retrieveSourceMap: retrieveSourceMap, + // For now, don't fix the source line in uncaught exceptions, because we + // haven't fixed handleUncaughtExceptions in source-map-support to properly + // locate the source files. + handleUncaughtExceptions: false +}); + + var files = exports; _.extend(exports, { // A sort comparator to order files into load order. @@ -595,12 +616,12 @@ _.extend(exports, { return future.wait(); }, - // Return the result of evaluating `code` using - // `runInThisContext`. `code` will be wrapped in a closure. You can - // pass additional values to bind in the closure in `env`, the keys - // being the symbols to bind and the values being their - // values. `filename` is the filename to use in exceptions that come - // from inside this code. + // Return the result of evaluating `code` using `runInThisContext`. `code` + // will be wrapped in a closure. You can pass additional values to bind in the + // closure in `options.symbols`, the keys being the symbols to bind and the + // values being their values. `options.filename` is the filename to use in + // exceptions that come from inside this code. `options.sourceMap` is an + // optional source map that represents the file. // // The really special thing about this function is that if a parse // error occurs, we will raise an exception of type @@ -612,19 +633,51 @@ _.extend(exports, { // instead -- only if the parse does in fact fail, to determine the // error we start a subprocess, redirect its stderr, grab the output // and parse it. - runJavaScript: function (code, filename, env) { + runJavaScript: function (code, options) { + if (typeof code !== 'string') + throw new Error("code must be a string"); + + options = options || {}; + var filename = options.filename || "<anonymous>"; var keys = [], values = []; // don't assume that _.keys and _.values are guaranteed to // enumerate in the same order - for (var k in env) { - keys.push(k); - values.push(env[k]); + _.each(options.symbols, function (value, name) { + keys.push(name); + values.push(value); + }); + + var stackFilename = filename; + if (options.sourceMap) { + // We want to generate an arbitrary filename that we use to associate the + // file with its source map. + stackFilename = "<runJavaScript-" + nextStackFilenameCounter++ + ">"; } - var header = "(function(" + keys.join(',') + "){"; + var chunks = []; + chunks.push("(function(" + keys.join(',') + "){"); + if (options.sourceMap) { + var consumer = new sourcemap.SourceMapConsumer(options.sourceMap); + chunks.push(sourcemap.SourceNode.fromStringWithSourceMap( + code, consumer)); + } else { + chunks.push(code); + } // \n is necessary in case final line is a //-comment - var footer = "\n})"; - var wrapped = header + code + footer; + chunks.push("\n})"); + + var wrapped; + if (options.sourceMap) { + var node = new sourcemap.SourceNode(null, null, null, chunks); + var results = node.toStringWithSourceMap({ + file: stackFilename + }); + wrapped = results.code; + var parsedSourceMap = results.map.toJSON(); + parsedSourceMaps[stackFilename] = parsedSourceMap; + } else { + wrapped = chunks.join(''); + }; try { // See #runInThisContext @@ -636,7 +689,7 @@ _.extend(exports, { // // Pass 'true' as third argument if we want the parse error on // stderr (which we don't.) - var func = require('vm').runInThisContext(wrapped, filename); + var func = require('vm').runInThisContext(wrapped, stackFilename); } catch (e) { // Got, presumably, a parse error. OK, we're going to start // another copy of node and feed it the offending code on @@ -685,7 +738,7 @@ SyntaxError: Unexpected identifier at Pipe.onread (net.js:418:51)" */ var err = new files.FancySyntaxError; - err.file = filename; + err.file = filename; // *not* stackFilename var lines = stderr.split('\n'); // line number @@ -709,6 +762,7 @@ SyntaxError: Unexpected identifier // adjust errors on line 1 to account for our header if (err.line === 1) { + // XXX XXX i killed header err.column -= header.length; err.columnEnd -= header.length; } diff --git a/tools/linker.js b/tools/linker.js index 42ecad44f4..b9e2c5f61d 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -60,6 +60,13 @@ _.extend(Module.prototype, { return _.max(maxInFile); }, + runLinkerFileTransforms: function (exports) { + var self = this; + _.each(self.files, function (f) { + f.runLinkerFileTransform(exports); + }); + }, + // Figure out which vars need to be specifically put in the module // scope. // @@ -70,7 +77,7 @@ _.extend(Module.prototype, { // and see what your globals are. Probably this means we need to // move the emission of the Package-scope Variables section (but not // the actual static analysis) to the final phase. - computeModuleScopeVars: function () { + computeModuleScopeVars: function (exports) { var self = this; if (!self.jsAnalyze) { @@ -78,7 +85,7 @@ _.extend(Module.prototype, { // js-analyze package. Let's do a stupid heuristic: any exports that have // no dots are module scope vars. (This works for // js-analyze.JSAnalyze...) - return _.filter(self.getExports(), function (e) { + return _.filter(exports, function (e) { return e.indexOf('.') === -1; }); } @@ -94,15 +101,13 @@ _.extend(Module.prototype, { }, // Output is a list of objects with keys 'source', 'servePath', 'sourceMap', - // 'sourcePath', 'source') - getPrelinkedFiles: function () { + // 'sourcePath' + getPrelinkedFiles: function (moduleExports) { var self = this; if (! self.files.length) return []; - var moduleExports = self.getExports(); - // If we don't want to create a separate scope for this module, // then our job is much simpler. And we can get away with // preserving the line numbers. @@ -230,8 +235,8 @@ var File = function (inputFile, module) { // A function which transforms the source code once all exports are // known. (eg, for CoffeeScript.) self.linkerFileTransform = - inputFile.linkerFileTransform || function (source, exports, sourceMap) { - return {source: source, sourceMap: sourceMap}; + inputFile.linkerFileTransform || function (sourceWithMap, exports) { + return sourceWithMap; }; // If true, don't wrap this individual file in a closure. @@ -297,6 +302,7 @@ _.extend(File.prototype, { // Recover by pretending that this file is empty (which // includes replacing its source code with '' in the output) self.source = ""; + self.sourceMap = null; return []; } }, @@ -313,6 +319,15 @@ _.extend(File.prototype, { return "app/" + self.sourcePath; }, + runLinkerFileTransform: function (exports) { + var self = this; + var sourceAndMap = self.linkerFileTransform( + {source: self.source, sourceMap: self.sourceMap}, + exports); + self.source = sourceAndMap.source; + self.sourceMap = sourceAndMap.sourceMap; + }, + // Options: // - preserveLineNumbers: if true, decorate minimally so that line // numbers don't change between input and output. In this case, @@ -327,14 +342,10 @@ _.extend(File.prototype, { // The newline after the source closes a '//' comment. if (options.preserveLineNumbers) { // Ugly version - // XXX XXX need to propagate source maps through linkerFileTransform! - var bodyWithMap = self.linkerFileTransform(self.source, options.exports, - self.sourceMap); - self.source = bodyWithMap.source; var mapNode; - if (bodyWithMap.sourceMap) { + if (self.sourceMap) { mapNode = sourcemap.SourceNode.fromStringWithSourceMap( - self.source, new sourcemap.SourceMapConsumer(bodyWithMap.sourceMap)); + self.source, new sourcemap.SourceMapConsumer(self.sourceMap)); } else { mapNode = new sourcemap.SourceNode(1, 0, self._pathForSourceMap(), self.source); @@ -345,8 +356,7 @@ _.extend(File.prototype, { return new sourcemap.SourceNode(null, null, null, [ self.bare ? "" : "(function(){", mapNode, - (self.source.length - && bodyWithMap.source[self.source.length - 1] !== '\n' + (self.source.length && self.source[self.source.length - 1] !== '\n' ? "\n" : ""), self.bare ? "" : "\n})();\n" ]); @@ -372,13 +382,6 @@ _.extend(File.prototype, { var blankLine = new Array(width + 1).join(' ') + " //\n"; chunks.push(blankLine); - var transformedSourceWithMap = self.linkerFileTransform( - self.source, options.exports, self.sourceMap); - // Update self.source, because that's what computeGlobalReferences will look - // at. - var originalSource = self.source; - self.source = transformedSourceWithMap.source; - // Code, with line numbers // You might prefer your line numbers at the beginning of the // line, with /* .. */. Well, that requires parsing the source for @@ -401,7 +404,7 @@ _.extend(File.prototype, { var lines = self.source.split('\n'); - if (transformedSourceWithMap.sourceMap) { + if (self.sourceMap) { var buf = ""; numberifyLines(function (line, suffix) { buf += line; @@ -415,7 +418,7 @@ _.extend(File.prototype, { // that important.) chunks.push(sourcemap.SourceNode.fromStringWithSourceMap( self.source, - new sourcemap.SourceMapConsumer(transformedSourceWithMap.sourceMap))); + new sourcemap.SourceMapConsumer(self.sourceMap))); } else { // There are probably ways to make a more compact source map. For example, // the only change we make is to append a comment, so we can probably emit @@ -439,8 +442,8 @@ _.extend(File.prototype, { // source in the source map. (If we are working on generated code, the // source map we received should have already contained the original // source.) - if (!transformedSourceWithMap.sourceMap) - node.setSourceContent(self._pathForSourceMap(), originalSource); + if (!self.sourceMap) + node.setSourceContent(self._pathForSourceMap(), self.source); return node; }, @@ -608,9 +611,18 @@ var prelink = function (options) { module.addFile(inputFile); }); - var files = module.getPrelinkedFiles(); + // 1) Figure out what this entire module exports. + // 2) Run the linkerFileTransforms, which depend on the exports. (This is, eg, + // CoffeeScript arranging to not close over the exports.) + // 3) Do static analysis to compute module-scoped variables; this has to be + // done based on the *output* of the transforms. Error recovery from the + // static analysis mutates the sources, so this has to be done before + // concatenation. + // 4) Finally, concatenate. var exports = module.getExports(); - var packageScopeVariables = module.computeModuleScopeVars(); + module.runLinkerFileTransforms(exports); + var packageScopeVariables = module.computeModuleScopeVars(exports); + var files = module.getPrelinkedFiles(exports); return { files: files, diff --git a/tools/packages.js b/tools/packages.js index 5266e4432b..898757617f 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -1317,8 +1317,10 @@ _.extend(Package.prototype, { }; try { - files.runJavaScript(code.toString('utf8'), 'package.js', - { Package: Package, Npm: Npm }); + files.runJavaScript(code.toString('utf8'), { + filename: 'package.js', + symbols: { Package: Package, Npm: Npm } + }); } catch (e) { buildmessage.exception(e); From 3205e33537153f35211dedaeae4abe9a846aeac3 Mon Sep 17 00:00:00 2001 From: David Glasser <glasser@meteor.com> Date: Thu, 11 Jul 2013 15:21:08 -0700 Subject: [PATCH 44/49] using source maps in JSImage.load now probably works --- scripts/generate-dev-bundle.sh | 4 + tools/buildmessage.js | 9 +- tools/files.js | 155 +++++++++++++++------------------ tools/linker.js | 3 +- tools/packages.js | 4 +- 5 files changed, 83 insertions(+), 92 deletions(-) diff --git a/scripts/generate-dev-bundle.sh b/scripts/generate-dev-bundle.sh index da49a6d517..c97eb65a37 100755 --- a/scripts/generate-dev-bundle.sh +++ b/scripts/generate-dev-bundle.sh @@ -110,6 +110,10 @@ npm install kexec@0.1.1 npm install shell-quote@0.0.1 npm install byline@2.0.3 +# Using the unreleased 1.1 branch. We can probably switch to a built NPM version +# when it gets released. +npm install https://github.com/ariya/esprima/tarball/5044b87f94fb802d9609f1426c838874ec2007b3 + # Fork of source-map which fixes one function with empty maps. # https://github.com/mozilla/source-map/pull/70 # See also below, where we get it into source-map-support. diff --git a/tools/buildmessage.js b/tools/buildmessage.js index 650d87787c..db2e8b84ac 100644 --- a/tools/buildmessage.js +++ b/tools/buildmessage.js @@ -356,8 +356,15 @@ var error = function (message, options) { // actually occurred, rather than the place where the exception was // thrown. var exception = function (error) { - if (! currentJob) + if (! currentJob) { + // XXX this may be the wrong place to do this, but it makes syntax errors in + // files loaded via unipackage.load have context. + if (error instanceof files.FancySyntaxError) { + error.message = "Syntax error: " + error.message + " at " + + error.file + ":" + error.line + ":" + error.column; + } throw error; + } var message = error.message; diff --git a/tools/files.js b/tools/files.js index 6787ff5e85..1e43df7935 100644 --- a/tools/files.js +++ b/tools/files.js @@ -623,16 +623,13 @@ _.extend(exports, { // exceptions that come from inside this code. `options.sourceMap` is an // optional source map that represents the file. // - // The really special thing about this function is that if a parse - // error occurs, we will raise an exception of type - // files.FancySyntaxError, from which you may read 'message', 'file', - // 'line', 'column', and 'columnEnd' attributes ... v8 is - // normally reluctant to reveal this information but will write it - // to stderr if you pass it an undocumented flag. Unforunately - // though node doesn't have dup2 so we can't intercept the write. So - // instead -- only if the parse does in fact fail, to determine the - // error we start a subprocess, redirect its stderr, grab the output - // and parse it. + // The really special thing about this function is that if a parse error + // occurs, we will raise an exception of type files.FancySyntaxError, from + // which you may read 'message', 'file', 'line', and 'column' attributes + // ... v8 is normally reluctant to reveal this information but will write it + // to stderr if you pass it an undocumented flag. Unforunately though node + // doesn't have dup2 so we can't intercept the write. So instead we use a + // completely different parser with a better error handling API. Ah well. runJavaScript: function (code, options) { if (typeof code !== 'string') throw new Error("code must be a string"); @@ -655,7 +652,8 @@ _.extend(exports, { } var chunks = []; - chunks.push("(function(" + keys.join(',') + "){"); + var header = "(function(" + keys.join(',') + "){"; + chunks.push(header); if (options.sourceMap) { var consumer = new sourcemap.SourceMapConsumer(options.sourceMap); chunks.push(sourcemap.SourceNode.fromStringWithSourceMap( @@ -667,15 +665,17 @@ _.extend(exports, { chunks.push("\n})"); var wrapped; + var parsedSourceMap = null; if (options.sourceMap) { var node = new sourcemap.SourceNode(null, null, null, chunks); var results = node.toStringWithSourceMap({ file: stackFilename }); wrapped = results.code; - var parsedSourceMap = results.map.toJSON(); + parsedSourceMap = results.map.toJSON(); parsedSourceMaps[stackFilename] = parsedSourceMap; } else { + wrapped = chunks.join(''); }; @@ -689,94 +689,75 @@ _.extend(exports, { // // Pass 'true' as third argument if we want the parse error on // stderr (which we don't.) - var func = require('vm').runInThisContext(wrapped, stackFilename); - } catch (e) { - // Got, presumably, a parse error. OK, we're going to start - // another copy of node and feed it the offending code on - // stdin. It should give us the error on stderr. + var script = require('vm').createScript(wrapped, stackFilename); + } catch (nodeParseError) { + if (!(nodeParseError instanceof SyntaxError)) + throw nodeParseError; + // Got a parse error. Unfortunately, we can't actually get the location of + // the parse error from the SyntaxError; Node has some hacky support for + // displaying it over stderr if you pass an undocumented third argument to + // stackFilename, but that's not what we want. See + // https://github.com/joyent/node/issues/3452 + // for more information. One thing to try (and in fact, what an early + // version of this function did) is to actually fork a new node + // to run the code and parse its output. We instead run an entirely + // different JS parser, from the esprima project, but which at least + // has a nice API for reporting errors. + var esprima = require('esprima'); + try { + esprima.parse(wrapped); + } catch (esprimaParseError) { + // Is this actually an Esprima syntax error? + if (!('index' in esprimaParseError && + 'lineNumber' in esprimaParseError && + 'column' in esprimaParseError && + 'description' in esprimaParseError)) { + throw esprimaParseError; + } + var err = new files.FancySyntaxError; - // Why do we think it has to be a parse error? After all, this is - // *run*InThisContext so it could be a thrown exception. But the only code - // we are running is the function definition, which should not be able to - // throw... unless "code" is imbalanced (ie, has a '})' in it), in which - // case "code" should basically be considered as a parse error - // anyway. (The error message in the latter case may be poor, though.) + err.message = esprimaParseError.description; - // XXX XXX don't run the code until after this block + if (parsedSourceMap) { + // XXX this duplicates code in computeGlobalReferences + var consumer2 = new sourcemap.SourceMapConsumer(parsedSourceMap); + var original = consumer2.originalPositionFor({ + line: esprimaParseError.lineNumber, + column: esprimaParseError.column - 1 + }); + if (original.source) { + err.file = original.source; + err.line = original.line; + err.column = original.column + 1; + throw err; + } + } - var Future = require('fibers/future'); - var future = new Future; - - var child_process = require("child_process"); - var proc = child_process.execFile( - process.argv[0], [], { - stdio: ['pipe'] - }, function (error, stdout, stderr) { - if (! error || error.code === 0) - future.return(null); // huh? didn't fail? - else - future.return(stderr); - }); - proc.stdin.write(wrapped); - proc.stdin.end(); - var stderr = future.wait(); - - if (stderr === null) - throw new Error("subprocess parsed bad code successfully?"); - -/* stderr will look something like this (note leading blank line:) -" -[stdin]:1 -couaoeua aaaaaa nsolexloeuaoeuao - ^^^^^^ -SyntaxError: Unexpected identifier - at Object.<anonymous> ([stdin]-wrapper:6:22) - at Module._compile (module.js:449:26) - at evalScript (node.js:282:25) - at Socket.<anonymous> (node.js:152:11) - at Socket.EventEmitter.emit (events.js:93:17) - at Pipe.onread (net.js:418:51)" -*/ - var err = new files.FancySyntaxError; - err.file = filename; // *not* stackFilename - var lines = stderr.split('\n'); - - // line number - var m = lines[1].match(/:(\d+)\s*$/); - if (! m) - throw new Error("can't parse line number from '" + lines[1] + "'"); - err.line = +m[1]; - - // column range - m = lines[3].match(/^(\s*)(\^+)\s*$/) - if (! m) - throw new Error("can't parse column indicator from '" + lines[3] + "'"); - err.column = m[1].length + 1; - err.columnEnd = err.column + m[2].length - 1; - - // message - m = lines[4].match(/^SyntaxError:\s*(.*)$/); - if (! m) - throw new Error("can't parse error message from '" + lines[4] + "'"); - err.message = m[1]; - - // adjust errors on line 1 to account for our header - if (err.line === 1) { - // XXX XXX i killed header - err.column -= header.length; - err.columnEnd -= header.length; + err.file = filename; // *not* stackFilename + err.line = esprimaParseError.lineNumber; + err.column = esprimaParseError.column; + // adjust errors on line 1 to account for our header + if (err.line === 1) { + err.column -= header.length; + } + throw err; } - throw err; + // What? Node thought that this was a parse error and esprima didn't? Eh, + // just throw Node's error and don't care too much about the line numbers + // being right. + throw nodeParseError; } + var func = script.runInThisContext(); + return (buildmessage.markBoundary(func)).apply(null, values); }, // - message: an error message from the parser + // - file: filename // - line: 1-based // - column: 1-based - // - columnEnd: 1-based FancySyntaxError: function () {}, OfflineError: function (error) { diff --git a/tools/linker.js b/tools/linker.js index b9e2c5f61d..25cfa78641 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -283,8 +283,7 @@ _.extend(File.prototype, { var errorOptions = { file: self.sourcePath, line: e.lineNumber, - column: e.column, - downcase: true + column: e.column }; if (self.sourceMap) { var parsed = new sourcemap.SourceMapConsumer(self.sourceMap); diff --git a/tools/packages.js b/tools/packages.js index 898757617f..6cac1c0920 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -316,11 +316,11 @@ _.extend(Slice.prototype, { // Assets.getText or Assets.getBinary. // - error({ message: "There's a problem in your source file", // sourcePath: "src/my/program.ext", line: 12, - // column: 20, columnEnd: 25, func: "doStuff" }) + // column: 20, func: "doStuff" }) // Flag an error -- at a particular location in a source // file, if you like (you can even indicate a function name // to show in the error, like in stack traces.) sourcePath, - // line, column, columnEnd, and func are all optional. + // line, column, and func are all optional. // // XXX for now, these handlers must only generate portable code // (code that isn't dependent on the arch, other than 'browser' From 4308b7c063fb308f50ff6ee85676d07a6b7fa3be Mon Sep 17 00:00:00 2001 From: David Glasser <glasser@meteor.com> Date: Thu, 11 Jul 2013 16:59:46 -0700 Subject: [PATCH 45/49] - switch from X-SourceMap header to //@ comment - specify sourceMapUrl in browser manifest - don't make source maps for app pure-JS files - make URLs happy - break traceback beauty --- packages/webapp/webapp_server.js | 108 +++++++++++-------------------- tools/bundler.js | 33 ++++++++-- tools/linker.js | 24 +++++-- tools/packages.js | 4 +- 4 files changed, 85 insertions(+), 84 deletions(-) diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js index 6453c9536d..f39dfcc134 100644 --- a/packages/webapp/webapp_server.js +++ b/packages/webapp/webapp_server.js @@ -215,8 +215,8 @@ var runWebAppServer = function () { next(); } }); - // Parse the query string into res.query. Used to detect sourcemap=1 and by - // oauth_server. + // Parse the query string into res.query. Used by oauth_server, but it's + // generally pretty handy.. app.use(connect.query()); // Auto-compress any json, javascript, or text. @@ -225,31 +225,19 @@ var runWebAppServer = function () { var staticFiles = {}; _.each(clientJson.manifest, function (item) { if (item.url && item.where === "client") { - var staticFile = { + staticFiles[url.parse(item.url).pathname] = { path: item.path, cacheable: item.cacheable }; - // Serve the source map too, under a hashed URL. Note that the hash is - // based on item.url which contains the file's hash, so this should change - // when the file changes and thus be cacheable. The URL ends with a slash, - // so that source files referenced from the source map with relative URLs - // are resolved under it. + // Serve the source map too, under the specified URL. We assume all source + // maps are cacheable. if (item.sourceMap) { - var sourceMapUrl = item.url; - var argument = "sourcemap=y"; - if (sourceMapUrl.indexOf('?') === -1) - sourceMapUrl += '?' + argument; - else - sourceMapUrl += '&' + argument; - - // Send the SourceMap header when the source file is served. - staticFile.urlToSourceMap = sourceMapUrl; - - // Register the source map itself to be served. - staticFile.sourceMapFile = item.sourceMap; + staticFiles[url.parse(item.sourceMapUrl).pathname] = { + path: item.sourceMap, + cacheable: true + }; } - staticFiles[url.parse(item.url).pathname] = staticFile; } }); @@ -279,18 +267,6 @@ var runWebAppServer = function () { var info = staticFiles[pathname]; - var fileToServe, urlToSourceMap; - if (req.query.sourcemap === 'y') { - // This is a request for a sourcemap. - fileToServe = info.sourceMapFile; - // Source maps don't have source maps. - urlToSourceMap = null; - } else { - // This is not a request for a sourcemap. - fileToServe = info.path; - urlToSourceMap = info.urlToSourceMap; - } - // Cacheable files are files that should never change. Typically // named by their hash (eg meteor bundled js and css files). // We cache them ~forever (1yr). @@ -302,48 +278,38 @@ var runWebAppServer = function () { // to include a query parameter to bust caches. That way we can both get // good caching behavior and allow users to change assets without delay. // https://github.com/meteor/meteor/issues/773 - // - // Because a source map's URL is constructed from the URL of the file it is - // the source map of, we assume that a source map is cacheable iff the file - // is cacheable. var maxAge = info.cacheable ? 1000 * 60 * 60 * 24 * 365 : 1000 * 60 * 60 * 24; - // Tell the client where to find the source map for this file. - if (urlToSourceMap) { - // This should just be SourceMap, but slightly more versions of Chrome - // support the older X-SourceMap. - // - // To figure out if your version of Chrome should support SourceMap, - // - go to chrome://version. Let's say the Chrome version is - // 28.0.1500.71 and the Blink version is 537.36 (@153022) - // - go to http://src.chromium.org/viewvc/blink/branches/chromium/1500/Source/core/inspector/InspectorPageAgent.cpp?view=log - // where the "1500" is the third part of your Chrome version - // - find the first revision that is no greater than the "153022" - // number. That's probably the first one and it probably has - // a message of the form "Branch 1500 - blink@r149738" - // - If *that* revision number (149738) is at least 151755, - // then Chrome should support SourceMap (not just X-SourceMap) - // (The change is https://codereview.chromium.org/15832007) - // - // You also need to enable source maps in Chrome: open dev tools, click - // the gear in the bottom right corner, and select "enable source maps". - // - // Firefox 23+ supports source maps (and they are on by default in 24+), - // but doesn't support either header yet: - // https://bugzilla.mozilla.org/show_bug.cgi?id=765993 - // We could make FF work by adding a comment to the end of the source map - // file. But that would require doing one of the following: - // - determining the source map URL at bundle time instead of here - // - writing to the source directory - // - not using the send module (or hacking it to allow a footer) - // None of these alternatives are great, so for now we just hope that - // FF will implement one of the headers soon. - res.setHeader('X-SourceMap', urlToSourceMap); - } + // It sure would be nice to write this here: + // res.setHeader('X-SourceMap', urlToSourceMap); + // Or even just SourceMap, but slightly more versions of Chrome support the + // older X-SourceMap. + // + // But we'd like to support FF, so for now we just use the "//@" comment at + // the end of the source map file. + // + // To figure out if your version of Chrome should support SourceMap, + // - go to chrome://version. Let's say the Chrome version is + // 28.0.1500.71 and the Blink version is 537.36 (@153022) + // - go to http://src.chromium.org/viewvc/blink/branches/chromium/1500/Source/core/inspector/InspectorPageAgent.cpp?view=log + // where the "1500" is the third part of your Chrome version + // - find the first revision that is no greater than the "153022" + // number. That's probably the first one and it probably has + // a message of the form "Branch 1500 - blink@r149738" + // - If *that* revision number (149738) is at least 151755, + // then Chrome should support SourceMap (not just X-SourceMap) + // (The change is https://codereview.chromium.org/15832007) + // + // You also need to enable source maps in Chrome: open dev tools, click + // the gear in the bottom right corner, and select "enable source maps". + // + // Firefox 23+ supports source maps (and they are on by default in 24+), + // but doesn't support either header yet: + // https://bugzilla.mozilla.org/show_bug.cgi?id=765993 - send(req, path.join(clientDir, fileToServe)) + send(req, path.join(clientDir, info.path)) .maxage(maxAge) .hidden(true) // if we specified a dotfile in the manifest, serve it .on('error', function (err) { @@ -352,7 +318,7 @@ var runWebAppServer = function () { res.end(); }) .on('directory', function () { - Log.error("Unexpected directory " + fileToServe); + Log.error("Unexpected directory " + info.path); res.writeHead(500); res.end(); }) diff --git a/tools/bundler.js b/tools/bundler.js index 096999e991..1494db8bea 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -303,6 +303,15 @@ _.extend(File.prototype, { return encoding ? self._contents.toString(encoding) : self._contents; }, + setContents: function (b) { + var self = this; + if (!(b instanceof Buffer)) + throw new Error("Must set contents to a Buffer"); + self._contents = b; + // Un-cache hash. + self._hash = null; + }, + size: function () { var self = this; return self.contents().length; @@ -863,16 +872,14 @@ _.extend(ClientTarget.prototype, { // Build up a manifest of all resources served via HTTP. var manifest = []; eachResource(function (file, type) { - writeFile(file, builder); + var fileContents = file.contents(); var manifestItem = { path: file.targetPath, where: "client", type: type, cacheable: file.cacheable, - url: file.url, - size: file.size(), - hash: file.hash() + url: file.url }; if (file.sourceMap) { @@ -886,8 +893,26 @@ _.extend(ClientTarget.prototype, { var mapData = new Buffer(")]}'\n" + file.sourceMap, 'utf8'); manifestItem.sourceMap = builder.writeToGeneratedFilename( file.targetPath + '.map', {data: mapData}); + + // Use a SHA to make this cacheable. + var sourceMapBaseName = file.hash() + ".map"; + // XXX When we can, drop all of this and just use the SourceMap + // header. FF doesn't support that yet, though. + // https://bugzilla.mozilla.org/show_bug.cgi?id=765993 + file.setContents(Buffer.concat([ + file.contents(), + new Buffer("\n//@ sourceMappingURL=" + sourceMapBaseName + "\n") + ])); + manifestItem.sourceMapUrl = require('url').resolve( + file.url, sourceMapBaseName); } + // Set this now, in case we mutated the file's contents. + manifestItem.size = file.size(); + manifestItem.hash = file.hash(); + + writeFile(file, builder); + manifest.push(manifestItem); }); diff --git a/tools/linker.js b/tools/linker.js index 25cfa78641..96b7bdab16 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -119,10 +119,17 @@ _.extend(Module.prototype, { file: file.servePath }); // results has 'code' and 'map' attributes + var sourceMap = results.map.toJSON(); + // No use generating empty source maps. + if (_.isEmpty(sourceMap.sources)) + sourceMap = null; + else + sourceMap = JSON.stringify(sourceMap); + return { source: results.code, servePath: file.servePath, - sourceMap: results.map.toString() + sourceMap: sourceMap }; }); } @@ -313,9 +320,9 @@ _.extend(File.prototype, { var self = this; if (self.module.name) - return "packages/" + self.module.name + "/" + self.sourcePath; + return self.module.name + "/" + self.sourcePath; else - return "app/" + self.sourcePath; + return require('path').basename(self.sourcePath); }, runLinkerFileTransform: function (exports) { @@ -346,10 +353,13 @@ _.extend(File.prototype, { mapNode = sourcemap.SourceNode.fromStringWithSourceMap( self.source, new sourcemap.SourceMapConsumer(self.sourceMap)); } else { - mapNode = new sourcemap.SourceNode(1, 0, self._pathForSourceMap(), - self.source); - // Provide the original content as well. - mapNode.setSourceContent(self._pathForSourceMap(), self.source); + // This is an app file that was always JS. The output file here is going + // to be the same name as the input file (because _pathForSourceMap in + // apps is the basename of the source file), and having a JS file + // pointing to a source map pointing to a JS file of the same name will + // (a) be confusing (b) be unnecessary since we aren't renumbering + // anything and (c) confuse at least Chrome. + mapNode = self.source; } return new sourcemap.SourceNode(null, null, null, [ diff --git a/tools/packages.js b/tools/packages.js index 6cac1c0920..46ac0f3bc8 100644 --- a/tools/packages.js +++ b/tools/packages.js @@ -361,8 +361,8 @@ _.extend(Slice.prototype, { // XXX duplicates _pathForSourceMap() in linker pathForSourceMap: ( self.pkg.name - ? "packages/" + self.pkg.name + "/" + relPath - : "app/" + relPath), + ? self.pkg.name + "/" + relPath + : path.basename(relPath)), // null if this is an app. intended to be used for the sources // dictionary for source maps. packageName: self.pkg.name, From 6eb9cdb69f7a461934b9182b73a1d788e4bde0b3 Mon Sep 17 00:00:00 2001 From: David Glasser <glasser@meteor.com> Date: Thu, 11 Jul 2013 17:24:47 -0700 Subject: [PATCH 46/49] make it work in node too --- tools/bundler.js | 14 ++++++++++---- tools/linker.js | 2 +- tools/server/boot.js | 8 +++++++- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/tools/bundler.js b/tools/bundler.js index 1494db8bea..e342de8536 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -252,8 +252,10 @@ var File = function (options) { self.sourcePath = options.sourcePath; // If this file was generated, a sourceMap (as a string) with debugging - // information. Set with setSourceMap. + // information, as well as the "root" that paths in it should be resolved + // against. Set with setSourceMap. self.sourceMap = null; + self.sourceMapRoot = null; // Where this file is intended to reside within the target's // filesystem. @@ -387,12 +389,13 @@ _.extend(File.prototype, { }, // Set a source map for this File. sourceMap is given as a string. - setSourceMap: function (sourceMap) { + setSourceMap: function (sourceMap, root) { var self = this; if (typeof sourceMap !== "string") throw new Error("sourceMap must be given as a string"); self.sourceMap = sourceMap; + self.sourceMapRoot = root; } }); @@ -663,7 +666,7 @@ _.extend(Target.prototype, { } if (resource.type === "js" && resource.sourceMap) { - f.setSourceMap(resource.sourceMap); + f.setSourceMap(resource.sourceMap, path.dirname(relPath)); } self[resource.type].push(f); @@ -1123,6 +1126,7 @@ _.extend(JsImage.prototype, { item.targetPath + '.map', { data: new Buffer(item.sourceMap, 'utf8') } ); + loadItem.sourceMapRoot = item.sourceMapRoot; } load.push(loadItem); @@ -1195,6 +1199,7 @@ JsImage.readFromDisk = function (controlFilePath) { rejectBadPath(item.sourceMap); loadItem.sourceMap = fs.readFileSync( path.join(dir, item.sourceMap), 'utf8'); + loadItem.sourceMapRoot = item.sourceMapRoot; } ret.jsToLoad.push(loadItem); }); @@ -1225,7 +1230,8 @@ _.extend(JsImageTarget.prototype, { source: file.contents().toString('utf8'), nodeModulesDirectory: file.nodeModulesDirectory, staticDirectory: file.staticDirectory, - sourceMap: file.sourceMap + sourceMap: file.sourceMap, + sourceMapRoot: file.sourceMapRoot }); }); diff --git a/tools/linker.js b/tools/linker.js index 96b7bdab16..f3858cd91a 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -641,7 +641,7 @@ var prelink = function (options) { }; var SOURCE_MAP_INSTRUCTIONS_COMMENT = banner([ - "This file was compiled by the Meteor linker. You can view the original", + "This is a generated file. You can view the original", "source in your browser if your browser supports source maps.", "", "If you are using Chrome, open the Developer Tools and click the gear", diff --git a/tools/server/boot.js b/tools/server/boot.js index b4f0c8011f..1170ca9ff0 100644 --- a/tools/server/boot.js +++ b/tools/server/boot.js @@ -50,13 +50,19 @@ _.each(serverJson.load, function (fileInfo) { // source-map-support doesn't ever look at the sourcesContent field, so // there's no point in keeping it in memory. delete parsedSourceMap.sourcesContent; + var url; + if (fileInfo.sourceMapRoot) { + // Add the specified root to any root that may be in the file. + parsedSourceMap.sourceRoot = path.join( + fileInfo.sourceMapRoot, parsedSourceMap.sourceRoot || ''); + } parsedSourceMaps[fileInfo.path] = parsedSourceMap; } }); var retrieveSourceMap = function (pathForSourceMap) { if (_.has(parsedSourceMaps, pathForSourceMap)) - return {map: parsedSourceMaps[pathForSourceMap]}; + return { map: parsedSourceMaps[pathForSourceMap] }; return null; }; From 73478a49f0ba9619d0c07b210abff5158b079be8 Mon Sep 17 00:00:00 2001 From: David Glasser <glasser@meteor.com> Date: Thu, 11 Jul 2013 17:37:30 -0700 Subject: [PATCH 47/49] support sourceMapRoot in JSImage.load too --- tools/bundler.js | 3 ++- tools/files.js | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/tools/bundler.js b/tools/bundler.js index e342de8536..1e6b9fcb48 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -1068,7 +1068,8 @@ _.extend(JsImage.prototype, { files.runJavaScript(item.source.toString('utf8'), { filename: item.targetPath, symbols: env, - sourceMap: item.sourceMap + sourceMap: item.sourceMap, + sourceMapRoot: item.sourceMapRoot }); } catch (e) { buildmessage.exception(e); diff --git a/tools/files.js b/tools/files.js index 1e43df7935..5fec9d691d 100644 --- a/tools/files.js +++ b/tools/files.js @@ -673,6 +673,14 @@ _.extend(exports, { }); wrapped = results.code; parsedSourceMap = results.map.toJSON(); + if (options.sourceMapRoot) { + // Add the specified root to any root that may be in the file. + parsedSourceMap.sourceRoot = path.join( + options.sourceMapRoot, parsedSourceMap.sourceRoot || ''); + } + // source-map-support doesn't ever look at the sourcesContent field, so + // there's no point in keeping it in memory. + delete parsedSourceMap.sourcesContent; parsedSourceMaps[stackFilename] = parsedSourceMap; } else { From b9f343f8b4f07420a9e921614ba69f5ea3bc0856 Mon Sep 17 00:00:00 2001 From: David Glasser <glasser@meteor.com> Date: Fri, 12 Jul 2013 09:54:32 -0700 Subject: [PATCH 48/49] Add Firefox instructions. Change comment style to one which does not make FF print warnings. This breaks Chrome, so start setting the header again. --- packages/webapp/webapp_server.js | 30 ++++++++++++++++++------------ tools/bundler.js | 10 +++++++--- tools/linker.js | 7 ++++++- 3 files changed, 31 insertions(+), 16 deletions(-) diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js index f39dfcc134..3490d7eb09 100644 --- a/packages/webapp/webapp_server.js +++ b/packages/webapp/webapp_server.js @@ -227,12 +227,14 @@ var runWebAppServer = function () { if (item.url && item.where === "client") { staticFiles[url.parse(item.url).pathname] = { path: item.path, - cacheable: item.cacheable + cacheable: item.cacheable, + // Link from source to its map + sourceMapUrl: item.sourceMapUrl }; - // Serve the source map too, under the specified URL. We assume all source - // maps are cacheable. if (item.sourceMap) { + // Serve the source map too, under the specified URL. We assume all + // source maps are cacheable. staticFiles[url.parse(item.sourceMapUrl).pathname] = { path: item.sourceMap, cacheable: true @@ -282,15 +284,15 @@ var runWebAppServer = function () { ? 1000 * 60 * 60 * 24 * 365 : 1000 * 60 * 60 * 24; - // It sure would be nice to write this here: - // res.setHeader('X-SourceMap', urlToSourceMap); - // Or even just SourceMap, but slightly more versions of Chrome support the - // older X-SourceMap. + // Set the X-SourceMap header, which current Chrome understands. + // (The files also contain '//#' comments which FF 24 understands and + // Chrome doesn't understand yet.) // - // But we'd like to support FF, so for now we just use the "//@" comment at - // the end of the source map file. + // Eventually we should set the SourceMap header but the current version of + // Chrome and no version of FF supports it. // - // To figure out if your version of Chrome should support SourceMap, + // To figure out if your version of Chrome should support the SourceMap + // header, // - go to chrome://version. Let's say the Chrome version is // 28.0.1500.71 and the Blink version is 537.36 (@153022) // - go to http://src.chromium.org/viewvc/blink/branches/chromium/1500/Source/core/inspector/InspectorPageAgent.cpp?view=log @@ -305,9 +307,13 @@ var runWebAppServer = function () { // You also need to enable source maps in Chrome: open dev tools, click // the gear in the bottom right corner, and select "enable source maps". // - // Firefox 23+ supports source maps (and they are on by default in 24+), - // but doesn't support either header yet: + // Firefox 23+ supports source maps but doesn't support either header yet, + // so we include the '//#' comment for it: // https://bugzilla.mozilla.org/show_bug.cgi?id=765993 + // In FF 23 you need to turn on `devtools.debugger.source-maps-enabled` + // in `about:config` (it is on by default in FF 24). + if (info.sourceMapUrl) + res.setHeader('X-SourceMap', info.sourceMapUrl); send(req, path.join(clientDir, info.path)) .maxage(maxAge) diff --git a/tools/bundler.js b/tools/bundler.js index 1e6b9fcb48..80237fc8b4 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -900,11 +900,15 @@ _.extend(ClientTarget.prototype, { // Use a SHA to make this cacheable. var sourceMapBaseName = file.hash() + ".map"; // XXX When we can, drop all of this and just use the SourceMap - // header. FF doesn't support that yet, though. - // https://bugzilla.mozilla.org/show_bug.cgi?id=765993 + // header. FF doesn't support that yet, though: + // https://bugzilla.mozilla.org/show_bug.cgi?id=765993 + // Note: if we use the older '//@' comment, FF 24 will print a lot + // of warnings to the console. So we use the newer '//#' comment... + // which Chrome (28) doesn't support. So we also set X-SourceMap + // in webapp_server. file.setContents(Buffer.concat([ file.contents(), - new Buffer("\n//@ sourceMappingURL=" + sourceMapBaseName + "\n") + new Buffer("\n//# sourceMappingURL=" + sourceMapBaseName + "\n") ])); manifestItem.sourceMapUrl = require('url').resolve( file.url, sourceMapBaseName); diff --git a/tools/linker.js b/tools/linker.js index f3858cd91a..b6eaf7eab7 100644 --- a/tools/linker.js +++ b/tools/linker.js @@ -646,7 +646,12 @@ var SOURCE_MAP_INSTRUCTIONS_COMMENT = banner([ "", "If you are using Chrome, open the Developer Tools and click the gear", "icon in its lower right corner. In the General Settings panel, turn", - "on 'Enable source maps'." + "on 'Enable source maps'.", + "", + "If you are using Firefox 23, go to `about:config` and set the", + "`devtools.debugger.source-maps-enabled` preference to true.", + "(The preference should be on by default in Firefox 24; versions", + "older than 23 do not support source maps.)" ]); // Finish the linking. From 7b85bbf99a4c9899eda06251be2c5323d7e99d5c Mon Sep 17 00:00:00 2001 From: David Glasser <glasser@meteor.com> Date: Fri, 12 Jul 2013 10:13:02 -0700 Subject: [PATCH 49/49] Bump dev bundle version number to 11 for sourcemaps! --- meteor | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meteor b/meteor index 6be11413e7..58613553c4 100755 --- a/meteor +++ b/meteor @@ -1,6 +1,6 @@ #!/bin/bash -BUNDLE_VERSION=0.3.9-plus-sourcemap +BUNDLE_VERSION=0.3.11 # OS Check. Put here because here is where we download the precompiled # bundles that are arch specific.