From 4308b7c063fb308f50ff6ee85676d07a6b7fa3be Mon Sep 17 00:00:00 2001 From: David Glasser Date: Thu, 11 Jul 2013 16:59:46 -0700 Subject: [PATCH] - 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,