Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sourceMap option. #59

Merged
merged 2 commits into from Jul 19, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 49 additions & 0 deletions Gruntfile.js
Expand Up @@ -10,6 +10,8 @@

module.exports = function(grunt) {

var path = require('path');

// Project configuration.
grunt.initConfig({
jshint: {
Expand Down Expand Up @@ -78,6 +80,53 @@ module.exports = function(grunt) {
'tmp/overwrite': ['test/fixtures/banner2.js']
}
},
sourcemap_options: {
options: {
banner: '// banner\n// line in banner\n',
separator: '\n// line in separator\n',
footer: '\n// line in footer\n// footer',
sourceMap: true,
sourceMapStyle: 'inline'
},
files: {
'tmp/sourcemap_inline': [
'test/fixtures/file0',
'test/fixtures/file2'
]
}
},
sourcemap2_options: {
options: {
sourceMap: true,
sourceMapName: function(dest) {
return path.join(
path.dirname(dest),
'maps',
path.basename(dest) + '.map'
);
},
sourceMapStyle: 'link'
},
files: {
'tmp/sourcemap2_link': [
'test/fixtures/mappedsource',
'test/fixtures/file2'
]
}
},
sourcemap3_options: {
options: {
sourceMap: true,
sourceMapName: 'tmp/sourcemap3_embed_map.map'
},
files: {
'tmp/sourcemap3_embed': [
'test/fixtures/mappedsource_embed',
'test/fixtures/file1',
'test/fixtures/file2'
]
}
}
},

// Unit tests.
Expand Down
18 changes: 18 additions & 0 deletions docs/concat-options.md
Expand Up @@ -50,3 +50,21 @@ _(Default processing options are explained in the [grunt.template.process][] doc

[templates]: https://github.com/gruntjs/grunt-docs/blob/master/grunt.template.md
[grunt.template.process]: https://github.com/gruntjs/grunt-docs/blob/master/grunt.template.md#grunttemplateprocess

## sourceMap
Type: `Boolean`
Default: `false`

Set to true to create a source map. The source map will be created alongside the destination file, and share the same file name with the `.map` extension appended to it.

## sourceMapName
Type: `String` `Function`
Default: `undefined`

To customize the name or location of the generated source map, pass a string to indicate where to write the source map to. If a function is provided, the concat destination is passed as the argument and the return value will be used as the file name.

## sourceMapStyle
Type: `String`
Default: `embed`

Determines the type of source map that is generated. The default value, `embed`, places the content of the sources directly into the map. `link` will reference the original sources in the map as links. `inline` will store the entire map as a data URI in the destination file.
3 changes: 2 additions & 1 deletion package.json
Expand Up @@ -27,7 +27,8 @@
"test": "grunt test"
},
"dependencies": {
"chalk": "~0.5.0"
"chalk": "~0.5.0",
"source-map": "~0.1.36"
},
"devDependencies": {
"grunt-contrib-jshint": "~0.10.0",
Expand Down
49 changes: 47 additions & 2 deletions tasks/concat.js
Expand Up @@ -13,6 +13,7 @@ module.exports = function(grunt) {
// Internal lib.
var comment = require('./lib/comment').init(grunt);
var chalk = require('chalk');
var sourcemap = require('./lib/sourcemap').init(grunt);
Copy link

Choose a reason for hiding this comment

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

Apologies if this feedback is undesirable, as you've done a lot of great work here.

I don't think it makes sense to even have an init method if you're going to run it automatically on require. It's sort of implied. To me your init method is more useful if you grab the module and only init if options.sourceMap is true, thus saving a bit of overhead. You'd have to do a bit of refactoring, but I can help if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brian-frichette thanks for the feedback.

On this specific point, I don't agree but I think I need some clarification. Are you against the init method or arguing against it being called init.

Overall this sounds like an unneeded micro-optimization. It may remove a little overhead for users who don't use source maps, but it would add issues when using the sourcemap helper library. Anything later in the file will need to check that the module is initialized and if not initialize it. In this case, it's not a big deal, but it sounds like a poor pattern to start.

Copy link

Choose a reason for hiding this comment

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

I agree on micro optimizations. I was just wondering aloud why you would even have a specific init method, then?

Copy link
Member

Choose a reason for hiding this comment

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

but it sounds like a poor pattern to start.

Agreed; I don't think optimizations are an issue here.

I was just wondering aloud why you would even have a specific init method, then?

It's just one of many patterns for making modules, I suppose. The other lib that this tasks utilizes has the same structure, so I'm for it if only for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason we code it with an init method is grunt tasks and related libs need to use the grunt module for the project being built instead of the grunt module that this library is dependent on during development. Those could be different versions or installed in different directories which would lead require('grunt') in the task or the lib/sourcemap.js file to return a different module than the one we need to define our task on or use functions like grunt.file.write.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or at least that is the theory that comes to mind. I'm not fully aware of the historical reasons in grunt for that.

Copy link
Member

Choose a reason for hiding this comment

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

grunt.log/file/util etc all rely on global state (verbose mode, dry run, etc). When grunt is invoked, it is passed directly into plugins so that their usage of these utilities respects those settings. I'm working to decouple these things with grunt-next but it's difficult. Basically, this is an architectural wart that we are stuck with for now.


grunt.registerMultiTask('concat', 'Concatenate files.', function() {
// Merge task-specific and/or target-specific options with these defaults.
Expand All @@ -21,7 +22,10 @@ module.exports = function(grunt) {
banner: '',
footer: '',
stripBanners: false,
process: false
process: false,
sourceMap: false,
sourceMapName: undefined,
sourceMapStyle: 'embed'
});
Copy link
Member

Choose a reason for hiding this comment

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

+      sourceMap: false,
+      sourceMapName: undefined,
+      sourceMapStyle: 'embed'

would it make sense to have

options: {
  sourceMap: true
},

and

options: {
  sourceMap: {
    name: 'tmp/sourcemap3_embed_map.map',
    style: ...
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When @jmeas and I were looking at standardizing sourceMap options in grunt-contrib we had a number of discussions about what those options should be. I remember arguing a similar way to configure deeper options. I don't recall the specifics of the conversation but we decided we wanted to keep the options flat and to only those three. The primary tenet for that decision was for simplicity for users and the code in grunt-contrib.

We wrote down the outcome of our decisions in a repo https://github.com/jmeas/sourcemap-options#sourcemap-options. It looks like we didn't store the logs of some of the better discussions but did comment on the pros/cons of those options. We figured more advanced settings past the three would be put into a task for modifying sourcemaps after the fact. Reflecting on that, one of the elements for the decision may have been that having only two options in the deeper object seemed a little silly and possibly misleading since there would be no others.

If you wish to expand on this, it'd be great to hear your thoughts on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope I'm not sounding dismissive. Just want to try and put some of the history behind the decisions for the current options here. I really would like to know your thoughts on it more if you would like to add them.

Copy link
Member

Choose a reason for hiding this comment

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

I do like the nested option as well, as it keeps the source map options all bundled together, which is a nice plus. The argument to keep it flat, if I remember right, was consistency with the majority of other tasks' options, which aren't nested.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure whether this is actually the case, but, nesting them might change the behaviour of overriding options for individual targets of the task.

They either will be or will not be merged with the other options. Either might be nice depending on the use case. Neither is significantly better or worse. The key is in documenting the behaviour well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Krinkle, that is a rather good point. I think https://github.com/gruntjs/grunt/blob/master/lib/grunt/task.js#L69 is the related line. It uses underscore's extend to perform the merge, so deep objects won't be merged.

I lean towards flat, but I don't feel strongly about it. I'm cool with deep objects for the options, but this point makes it trickier than a little change to this branch.

@jmeas @vladikoff thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also leaning towards flat

Copy link
Member

Choose a reason for hiding this comment

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

yeah seems like we don't have a choice


// Normalize boolean options that accept options objects.
Expand All @@ -32,8 +36,35 @@ module.exports = function(grunt) {
var banner = grunt.template.process(options.banner);
var footer = grunt.template.process(options.footer);

// Set a local variable for whether to build source maps or not.
var sourceMap = options.sourceMap;

// If content is not embedded and it will be modified, either exit or do
// not make the source map.
if (
sourceMap && options.sourceMapStyle === 'link' &&
(options.stripBanners || options.process)
) {
// Warn and exit if --force isn't set.
grunt.warn(
'stripBanners or process option is enabled. ' +
'Set sourceMapStyle option to \'embed\' or \'inline\'.'
);
// --force is set, continue on without the source map.
grunt.log.warn('Skipping creation of source maps.');
// Set sourceMap to false to keep maps from being constructed.
sourceMap = false;
}

// Iterate over all src-dest file pairs.
this.files.forEach(function(f) {
// Initialize source map objects.
var sourceMapHelper;
if (sourceMap) {
sourceMapHelper = sourcemap.helper(f, options);
sourceMapHelper.add(banner);
}

// Concat banner + specified files + footer.
var src = banner + f.src.filter(function(filepath) {
// Warn on and remove invalid source files (if nonull was set).
Expand All @@ -43,7 +74,7 @@ module.exports = function(grunt) {
} else {
return true;
}
}).map(function(filepath) {
}).map(function(filepath, i) {
if (grunt.file.isDir(filepath)) {
return;
}
Expand All @@ -59,9 +90,23 @@ module.exports = function(grunt) {
if (options.stripBanners) {
src = comment.stripBanner(src, options.stripBanners);
}
// Add the lines of this file to our map.
if (sourceMapHelper) {
src = sourceMapHelper.addlines(src, filepath);
if (i < f.src.length - 1) {
sourceMapHelper.add(options.separator);
}
}
return src;
}).join(options.separator) + footer;

if (sourceMapHelper) {
sourceMapHelper.add(footer);
sourceMapHelper.write();
// Add sourceMappingURL to the end.
src += sourceMapHelper.url();
}

// Write the destination file.
grunt.file.write(f.dest, src);

Expand Down
208 changes: 208 additions & 0 deletions tasks/lib/sourcemap.js
@@ -0,0 +1,208 @@
/*
* grunt-contrib-concat
* http://gruntjs.com/
*
* Copyright (c) 2013 "Cowboy" Ben Alman, contributors
* Licensed under the MIT license.
*/

'use strict';

exports.init = function(grunt) {
var exports = {};

// Node first party libs
var path = require('path');

// Third party libs
var chalk = require('chalk');
var SourceMapConsumer = require('source-map').SourceMapConsumer;
var SourceMapGenerator = require('source-map').SourceMapGenerator;
var SourceNode = require('source-map').SourceNode;

// Return an object that is used to track sourcemap data between calls.
exports.helper = function(files, options) {
// Figure out the source map destination.
var dest = files.dest;
if (options.sourceMapStyle === 'inline') {
// Leave dest as is. It will be used to compute relative sources.
} else if (typeof options.sourceMapName === 'string') {
dest = options.sourceMapName;
} else if (typeof options.sourceMapName === 'function') {
dest = options.sourceMapName(dest);
Copy link
Member

Choose a reason for hiding this comment

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

If this is in a try/catch we can throw a useful error if the user writes some messed up Javascript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. Any specific thoughts on what a useful error should be?

I figured running grunt concat --stack would be good enough, since it'd print the full stack, including the parts in the user side code.

Also grunt-contrib-concat doesn't do anything like that around the standard grunt process option. concat.js#L50

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I got the idea from grunt-contrib-uglify. I like the idea of consistent errors for this option, but you're right that --stack would likely be just as helpful. Whatever we go with should be used consistently across grunt-contrib for all functions input by the user, I think.

If we did decide to go with this I would just go with something simple, like: 'There was an error when generating the source map name' followed by the error. But I'm undecided on it.

Thoughts?

// cc @tkellen @vladikoff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I'd like a more general solution. Having a specific try-catch around every user function feels like something grunt's core could provide. I think the change I'd like to see is grunt core stating the --stack option on any error. Currently you get

Warning: Error message goes here. Use --force to continue.

Maybe instead have something like

Warning: Error message goes here. Use --force to continue or --stack to get debug info.

@tkellen, @vladikoff thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

@cowboy would be better equipped to answer this one.

} else {
dest = dest + '.map';
}

// Inline style and sourceMapName together doesn't work
if (options.sourceMapStyle === 'inline' && options.sourceMapName) {
grunt.log.warn(
'Source map will be inlined, sourceMapName option ignored.'
);
}

return new SourceMapConcatHelper({
files: files,
dest: dest,
options: options
});
};

function SourceMapConcatHelper(options) {
this.files = options.files;
this.dest = options.dest;
this.options = options.options;

// Create the source map node we'll add concat files into.
this.node = new SourceNode();

// Create an array to store source maps that are referenced from files
// being concatenated.
this.maps = [];
}

// Construct a node split by a zero-length regex.
SourceMapConcatHelper.prototype._dummyNode = function(src, name) {
var node = new SourceNode();
var lineIndex = 1;
var charIndex = 0;
var tokens = src.split(/\b/g);

tokens.forEach(function(token, i) {
node.add(new SourceNode(lineIndex, charIndex, name, token));
// Count lines and chars.
token.split('\n').forEach(function(subtoken, j, subtokens) {
if (j < subtokens.length - 1) {
lineIndex++;
charIndex = 0;
} else {
charIndex += subtoken.length;
}
});
});

return node;
};

// Add some arbitraty text to the sourcemap.
SourceMapConcatHelper.prototype.add = function(src) {
// Use the dummy node to track new lines and character offset in the unnamed
// concat pieces (banner, footer, separator).
this.node.add(this._dummyNode(src));
};

// Add the lines of a given file to the sourcemap. If in the file, store a
// prior sourcemap and return src with sourceMappingURL removed.
SourceMapConcatHelper.prototype.addlines = function(src, filename) {
var relativeFilename = path.relative(path.dirname(this.dest), filename);

var node;
if (
/\/\/[@#]\s+sourceMappingURL=(.+)/.test(src) ||
/\/\*#\s+sourceMappingURL=(\S+)\s+\*\//.test(src)
) {
var sourceMapFile = RegExp.$1;
var sourceMapPath;

var sourceContent;
// Browserify, as an example, stores a datauri at sourceMappingURL.
if (/data:application\/json;base64,([^\s]+)/.test(sourceMapFile)) {
// Set sourceMapPath to the file that the map is inlined.
sourceMapPath = filename;
sourceContent = new Buffer(RegExp.$1, 'base64').toString();
} else {
// If sourceMapPath is relative, expand relative to the file
// refering to it.
sourceMapPath = path.resolve(path.dirname(filename), sourceMapFile);
sourceContent = grunt.file.read(sourceMapPath);
}
var sourceMap = JSON.parse(sourceContent);
var sourceMapConsumer = new SourceMapConsumer(sourceMap);
// Consider the relative path from source files to new sourcemap.
var sourcePathToSourceMapPath =
path.relative(path.dirname(this.dest), path.dirname(sourceMapPath));
// Store the sourceMap so that it may later be consumed.
this.maps.push([
sourceMapConsumer, relativeFilename, sourcePathToSourceMapPath
]);
// Remove the old sourceMappingURL.
src = src.replace(/[@#]\s+sourceMappingURL=[^\s]+/, '');
// Create a node from the source map for the file.
node = SourceNode.fromStringWithSourceMap(
src, sourceMapConsumer, sourcePathToSourceMapPath
);
} else {
// Use a dummy node. Performs a rudimentary tokenization of the source.
node = this._dummyNode(src, relativeFilename);
}

this.node.add(node);

if (this.options.sourceMapStyle !== 'link') {
this.node.setSourceContent(relativeFilename, src);
}

return src;
};

// Return the comment sourceMappingURL that must be appended to the
// concatenated file.
SourceMapConcatHelper.prototype.url = function() {
// Create the map filepath. Either datauri or destination path.
var mapfilepath;
if (this.options.sourceMapStyle === 'inline') {
var inlineMap = new Buffer(this._write()).toString('base64');
mapfilepath = 'data:application/json;base64,' + inlineMap;
} else {
// Compute relative path to source map destination.
mapfilepath = path.relative(path.dirname(this.files.dest), this.dest);
}
// Create the sourceMappingURL.
var url;
if (/\.css$/.test(this.files.dest)) {
url = '\n/*# sourceMappingURL=' + mapfilepath + ' */';
} else {
url = '\n//# sourceMappingURL=' + mapfilepath;
}

return url;
Copy link
Member

Choose a reason for hiding this comment

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

This is semi-related to our discussions recently regarding methods operating on files/maps/both, but it strikes me as a bit odd that this method returns the url to append to the file and writes the source map. It seems to me that it might be doing too much. How do we handle the situation where it can't write the map for some reason (permission, etc.)? Would it still return the url?

The alternative is having more code in tasks/concat.js to make two separate calls, but it might make more sense in this situation.

Thoughts @mzgoddard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's related to the files/maps/both topic. I think this function got overloaded because of a few lines up. https://github.com/gruntjs/grunt-contrib-concat/pull/59/files#diff-c393a8ca95eaa9a57c99b11eaa6548f5R162 I believe was part of the source that this PR was originally in. Coding from that I arrived at a dilemma of how to wrangle that. Though it was a false dilemma. The map written to the file system doesn't need to know the part of it's source that is the comment referring to it. So yeah, definitely going to change this.

On the situation of cases like being unable to write a map to the file system, that wouldn't be improved by having a different architecture. That's an error and handling that is outside the scope of this file. It should boot the error up and something else figures it out. Currently grunt would stop and tell the user the error.

I'm not worried about having a few more lines in concat.js. It'd be nice to have this the functionality in that and here be split but it's highly related so that'd be hard to do without doing the possibly bad magic demo'd here.

};

// Return a string for inline use or write the source map to disk.
SourceMapConcatHelper.prototype._write = function() {
var code_map = this.node.toStringWithSourceMap({
file: path.relative(path.dirname(this.dest), this.files.dest)
});
// Consume the new sourcemap.
var generator = SourceMapGenerator.fromSourceMap(
new SourceMapConsumer(code_map.map.toJSON())
);
// Consume sourcemaps for source files.
this.maps.forEach(Function.apply.bind(generator.applySourceMap, generator));
// New sourcemap.
var newSourceMap = generator.toJSON();
// Return a string for inline use or write the map.
if (this.options.sourceMapStyle === 'inline') {
grunt.log.writeln(
'Source map for ' + chalk.cyan(this.files.dest) + ' inlined.'
);
return JSON.stringify(newSourceMap, null, '');
} else {
grunt.file.write(
this.dest,
JSON.stringify(newSourceMap, null, '')
);
grunt.log.writeln('Source map ' + chalk.cyan(this.dest) + ' created.');
}
};

// Non-private function to write the sourcemap. Shortcuts if writing a inline
// style map.
SourceMapConcatHelper.prototype.write = function() {
if (this.options.sourceMapStyle !== 'inline') {
this._write();
}
};

return exports;
};