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

Add sourceMap option. #59

merged 2 commits into from Jul 19, 2014

Conversation

mzgoddard
Copy link
Contributor

In response to issue #12.

The sourceMap option, when enabled writes a source map of the concatenated file. I started from @kozy4324's https://github.com/kozy4324/grunt-concat-sourcemap. It has further options to store the file content in the map, inline the map in the concatenated file, and an option stored in the source map for the root directory of the source files.

@Anahkiasen
Copy link

+1 for this.

@Driklyn
Copy link

Driklyn commented Jan 23, 2014

+1

3 similar comments
@cm0s
Copy link

cm0s commented Jan 29, 2014

+1

@igorzoriy
Copy link

👍

@andyearnshaw
Copy link

👍

@cm0s
Copy link

cm0s commented Feb 7, 2014

Great thank you!

@bfricka
Copy link

bfricka commented Feb 7, 2014

👍

@jamesplease
Copy link
Member

@mzgoddard, a source map option makes a lot of sense for this task – thanks for putting together this PR. Before merging I think we should go over the specifics of the implementation, though; I've recently begun a project to unify the source map options across all of the grunt contrib suite. I'll shoot you a message on IRC.

@bfricka
Copy link

bfricka commented Feb 11, 2014

Just wanted to poke my head in and see if there have been any stumbling blocks here. This is very logical option for this task as @jmeas mentioned, and I'd love to see it implemented.

The project to unify the source map options across *-contrib tasks is brilliant. The main concern for me would be less, since their source map implementation (less.js, I mean) is pretty rough. I'd hate to see the unification project hold up landing this PR, at least if the delay is due to fleshing out the semantics required to satisfy problematic source map implementations as mentioned.

Please let me know if I can assist.

@jamesplease
Copy link
Member

@brian-frichette, thanks for the support. Z and I are scheduled to talk today and get this figured out. I don't like to make promises, but seeing as he's already done most of the work (thanks again @mzgoddard), I imagine it will mostly just be a tweaking of the option names, and hardly any programmatic stuff. In other words, it will likely get done pretty quickly.

If you're in a real rush, and a few days is too long to wait, you could clone his branch and use that instead.

@bfricka
Copy link

bfricka commented Feb 11, 2014

No rush at all (we already have a hack in place). Just don't like to see quality PRs languish. :) So, mostly cheerleading.

Edit: By help I really meant on the *-contrib normalization stuff.

@mzgoddard
Copy link
Contributor Author

After talking with @jmeas about desired source map options for grunt-contrib-*, I'll have changes to this branch up in a few days.

@jamesplease
Copy link
Member

👍

@mzgoddard
Copy link
Contributor Author

@jmeas

I made these changes last night.

  1. Branch now follows config options from https://github.com/jmeas/sourcemap-options

  2. Branch computes correct relative paths (in sourceMappingURL and source map sources field)
    When I first wrote this branch based on @kozy4324's repo, I wasn't sure how to implement relative paths and removed what didn't work correctly at the time. Adding sourceMapName, I really wanted those to work. The second and (new) third test cases now test these relative urls.

    The second test, now storing the map in a maps directory, in particular demonstrates how the changes to the relative paths in the map's sources field. After I implemented relative paths when adding to the first map, subsequent concats that consumed the files that already had maps didn't update their sources field. Until I fixed it, the second test ended up with a duplicate file2 since its path for adding file2 a second time was different than the path consumed from the first map.

    I'm thinking supporting accurate relative paths will be one of the recurring jobs of supporting source maps through the grunt plugins. But, I'm hoping it won't be.

  3. Improved path handling in loading source map by sourceMappingURL
    I was previously using a crappy regex replacement to compute the full path to read a source map. Functionally, this helps computing the step above.

  4. Testing inline, embed, and link
    A test for each.

  5. Classy SourceMapConcatHelper
    I wasn't sure best how to architect the helper for supporting sourcemaps. Being an internal helper this currently works but I feel like whatever we take from this for generalizing it, can be made better.

@jamesplease
Copy link
Member

👍 Thanks, @mzgoddard. I'll look over this this weekend and hopefully get it merged.


// Continue on, but let the developer know that we are forcing embed.
grunt.log.warn('Executing as if sourceMapStyle was set to \'embed\'.');
options.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.

Hm, I think I'd rather something fail if the user chooses incompatible options rather than it adjusting things so that they work. There are two options for this:

  1. Don't generate the source map and keep going
  2. Fail the task fatally; don't even generate the dest file

Of these two options, I think the first makes the most sense. A good error message would be enough to let them know what they need to change to generate the source map that they want.

What do you think, @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.

That sounds fair. (2) is currently done with grunt's fail unless forced mechanic with grunt.warn. But the first sounds better to me if it is forced. I'll have a change up later today.

@mzgoddard
Copy link
Contributor Author

@jmeas Made two fixups for the points you brought up. 078c7c6 reworks sourcemap.js's helper to state that generating the source map will be skipped, instead of changing the style option. 91c8eeb makes the sourcemap concatenations standalone of each other. Let me know your thoughts.

(I can understand the mappings field now. O_O)

// --force is set, continue on without the source map.
grunt.log.warn('Skipping creation of source map ' + dest);

return new SourceMapConcatNoop();
Copy link
Member

Choose a reason for hiding this comment

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

If the conditionals above were moved to concat.js, then we could get rid of this SourceMapConcatNoop object. This changes the question from 'should I make a noop object or a real object?' to 'do I need a source map helper or not?' I think the second question seems a bit more elegant. What do you think, @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.

I'm indifferent about this. I pushed 04fa650 which makes your proposed change.

@jamesplease
Copy link
Member

Aside from the noop bit I think this is ready to be merged. Really nice work here, @mzgoddard.

@bfricka
Copy link

bfricka commented Feb 20, 2014

Congrats on all the hard work guys 👍

@jamesplease
Copy link
Member

Looks good to me. I'm going to do one more thorough look tomorrow before merging.

💃

@@ -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.

);
// Consume sourcemaps for source files.
this.maps.forEach(function(sourceMap){
generator.applySourceMap(new SourceMapConsumer(sourceMap));
Copy link

Choose a reason for hiding this comment

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

Use the second and third parameters of .applySourceMap here, as mentioned above.

@lydell
Copy link

lydell commented Mar 3, 2014

You seem to only support line numbers, not column numbers in the source map, by adding a mapping at the beginning of each line. For example, an error at concat.js:135:2, concat.js:135:20 or concat.js:135:200 will all map back to source2.js:5:0. Better than nothing, though. (Firefox doesn’t even support column numbers in its stack traces, so nothing lost there. But Chrome and Opera does.). If a source is minified it will be horrible, though. In that case, getting concat:135:200 is way better than source.min.js:1:0. Note that even if the minifier that was used produced a source map, it won’t help (see below). But perhaps you’re not supposed to minify before concatenating.

This line-numbers-only approach is also bad when the files to be concatenated are generated by preprocessors. Even though you read their source maps and applies them, it does not make much of a difference. Why? Because the .applySourceMap method works essentially like this:

  • It goes through each mapping of the source map (let’s call it map A).
  • For each mapping, it looks up the original line and column in the map to be applied (map B). If map B maps that location on to one of its sources, the mapping of map A is modified to use map B’s location instead. Otherwise the mapping of map A is left untouched.

Let’s think about what the output of a preprocessor such as Sass or CoffeeScript looks like: Basically it’s nice code that is supposed to look hand-written. Which for example means nice indentation. Usually, preprocessors do not map the indentation to the source (why would they—it might have nothing to do with the source at all!). Instead, the first mapping of each line will probably be at the first token after the indentation.

Your source map contains mappings only for column 0 of each line—that is, very likely inside indentation. So the .applySourceMap method won’t find any source locations in the preprocessor’s source map for most of your mappings—leaving most of them untouched!

Note that you might disagree with me about the above, since you having been using mozilla/source-map@<=0.1.32 which had a bug that was fixed in 0.1.33. Please try things out with the new version.

The above indicate that the tests need to be improved. They are currently too simplistic to catch the above behaviors. For example, the tests lack indented code. I’d suggest using some real JavaScript code and CSS code instead/too.

A better solution could be something like this:

  • If a source to concatenate already has a source map, copy all its mappings into our source map. Just increase the lines with the current offset.
  • If a source to concatenate does not have a source map ... here I see two ways to go:
    • Parse the source using a JS or CSS parser with source map support to get a source map. PostCSS, for example, parses and stringifies CSS with source map without changing a single byte of the code unless you want to. However, this might feel overkill. I also do not know of such a JS parser (but perhaps it doesn’t matter if the code style is altered, as long as the program is equivalent?).
    • Add more mappings at each line than at column 0. Perhaps .split(/\b/)ing the line and adding one mapping per segment?

Lastly, I have two more things to say:

  • A really big 👍 for the sensible and easy to understand source map options. I like how they are flexible, but still simple and enforcing good practice. The default source map style is great.
  • Isn’t this grunt plugin getting a bit complicated? This feels like a general concatenation-with-source-maps solution that could live as a separate module, consumed by this plugin.

@jamesplease
Copy link
Member

@lydell I can't thank you enough for all of these comments! This is immensely helpful.

This feels like a general concatenation-with-source-maps solution that could live as a separate module, consumed by this plugin.

We couldn't agree more. Much of this will be pulled out into a generalized, separate library for handling source maps. (note: the repo is a bit bare right now :) )

@mzgoddard
Copy link
Contributor Author

@lydell, Wow. Thank you for all this feedback! It might take a day or two
to respond to all of it.

On Mon, Mar 3, 2014 at 1:37 PM, Jmeas Smith notifications@github.comwrote:

@lydell https://github.com/lydell I can't thank you enough for all of
these comments! This is immensely helpful.

This feels like a general concatenation-with-source-maps solution that
could live as a separate module, consumed by this plugin.

We couldn't agree more. Much of this will be pulled out into a
generalized, separate library https://github.com/jmeas/sourcemap-utilfor handling source maps.

Reply to this email directly or view it on GitHubhttps://github.com//pull/59#issuecomment-36542751
.

@lydell
Copy link

lydell commented Mar 3, 2014

We couldn't agree more. Much of this will be pulled out into a generalized, separate library for handling source maps. (note: the repo is a bit bare right now :) )

Cool! (Shameless plug: I’m working on modules for handling source maps as well: source-map-url, source-map-resolve.)

var sourceMap = JSON.parse(sourceContent);
// The source map generated by sass 3.3.0.rc.1 seems to be wrong format.
if (typeof sourceMap.version === 'string') {
sourceMap.version = parseInt(sourceMap.version, 10);
Copy link

Choose a reason for hiding this comment

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

This is true, but why do we need to care?

@lydell
Copy link

lydell commented Mar 6, 2014

I think we should warn that you will likely break the source maps if source maps and the process option are combined.

Edit: Sorry, I missed that you already had thought about that. #2 (comment)

@lydell
Copy link

lydell commented Mar 9, 2014

I think the way to go is:

  1. Create dummy source maps for all files that haven’t already got source maps.
  2. Create a new source map.
  3. Copy all mappings in all other source maps in order into the new map, adjusting the generated line and source relativeness along the way.

@mzgoddard
Copy link
Contributor Author

Fixups for @lydell's simple feedback. I'll get a change up for the more complicated source map mappings issue he brought up soon.

@mzgoddard
Copy link
Contributor Author

Also note: the Travis CI fail is currently intentional. source-map needs a fix (mozilla/source-map#100) so that the third parameter honors source content from earlier maps. I think we should wait for that fix or another fix for that problem before merging this.

@lydell
Copy link

lydell commented Mar 22, 2014

I’ve created source-map-concat, which might be of help :)

@vladikoff
Copy link
Member

ping @mzgoddard

@Krinkle
Copy link
Contributor

Krinkle commented Jun 26, 2014

Poke. Would be great to see this land. https://github.com/kozy4324/grunt-concat-sourcemap has this already, but it doesn't have a banner option for instance.

@Krinkle Krinkle mentioned this pull request Jun 26, 2014
@Krinkle
Copy link
Contributor

Krinkle commented Jun 26, 2014

Fixed at #88.

@mzgoddard
Copy link
Contributor Author

  • Updated after the squash @Krinkle did in Add sourceMap option #88 (thanks again!).
  • Fixed the newline separator issue @ColCh brought up.
  • Bumped to source-map 0.1.36 And fixed the tests. source-map moved where it puts the file member and the current strict tests didn't dig that.
  • Improved the source node creation from a lossy transform of the source maps for input files to better letting source-map handle inclusion of past source maps. Also adds dummy map creation for files that don't have nodes. This uses the regex word separator to split the source into very basic word tokens. This isn't the most desirable, for something better though we'll want to depend on some lib that parses source and outputs maps that can be consumed by this in a better fashion but that parser would need to handle multiple types of source code (presumable at least javascript and css to be useful).

@mzgoddard
Copy link
Contributor Author

@ColCh I'm guessing you have a use case with the new lines in the separator would be great to know if my fix works for you. mzgoddard@98581d2 tests the separator fix (also banner and footer) but knowing it works with your use case would be great too.

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

@vladikoff
Copy link
Member

@mzgoddard Thanks for updating the PR. See my configuration comment above

mzgoddard and others added 2 commits July 18, 2014 00:00
- sourceMap enabled, will write a source map of the concatenated file.
- sourceMapName option controls the name of the exported source map.
  - sourceMapName can be a string or a function
- sourceMapStyle option can be `link`, `embed`, or `inline`
  - as `link`, source map will refer to concatenated files
  - as `embed`, source map will embed file contents
  - as `inline`, source map will be a data url in the concatenated file

If stripBanners or process is enabled, sourceMapStyle == embed or
inline is required. This way the source presented using the map will
be accurate.
This is a new method in Node v0.11.2 (Unstable) not
available in node v0.8 or v0.10 and v0.12 isn't out yet.

http://blog.nodejs.org/2013/05/13/node-v0-11-2-unstable/
nodejs/node-v0.x-archive@90266750617
@mzgoddard
Copy link
Contributor Author

I think this code is ready to go. I'd appreciate it if another Collaborator 👍 it if they think it's ready too. Then I'll merge.

@vladikoff @jmeas

@vladikoff
Copy link
Member

@mzgoddard I'll check this tomorrow morning. However @jmeas feel free to also check it :)

@vladikoff vladikoff merged commit 4186f06 into gruntjs:master Jul 19, 2014
Krinkle added a commit to wikimedia/oojs-ui that referenced this pull request Oct 26, 2015
We switched to a fork of mine last year because of a bug (upstream
started to depend on unstable APIs not released in the version we have)
and because of missing Source Maps support.

Both have been merged and released upstream.

gruntjs/grunt-contrib-concat#59
https://github.com/gruntjs/grunt-contrib-concat/commits/v0.5.0

Bug: T116594
Change-Id: I9093fc4633106bfc121f66835e411cc09474712e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet