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

Already on GitHub? Sign in to your account

Source maps are not usable when the banner option is used #22

Closed
dylang opened this Issue Feb 6, 2013 · 14 comments

Comments

Projects
None yet
9 participants
Contributor

dylang commented Feb 6, 2013

The banner option appends lines to the file which change the line numbering that was used by the source map module. This results in unusable source maps.

I'm not sure what the right fix is, here are some thoughts:

  • When both options are used warn the user the the source map will not be essentially broken and unusable.
  • When both options are used warn the user that the banner option is being ignored so that the source maps will work.
  • Figure out if Uglify has a banner option that is compatible with source maps and use that instead.
  • File a ticket or PR with uglify to support this feature.

Note: I haven't researched the Uglify side of things to see if maybe there is an Uglify option for this already that we should be using.

Member

jsoverson commented Feb 7, 2013

Uglification (and this task) supports the retention of certain style comments which are often used for licenses, but the banner option is appended after uglification which bypasses that.

If you're prepending a license as a comment starting with a bang (/*! foo */), you can include it as the first option in your source and turn on partial preservation of comments.

yourTask : {
  files : {
    'dest.js' : ['src/license','src/**/*.js']
    options : {
      preserveComments : 'some'
    }
  }
}

(with all your sourcemap options, obviously)

Not sure what the best route is, whether this should be something handle by Uglify, whether this task should be smarter, or if this option should be the recommended route.

Contributor

dylang commented Feb 8, 2013

@jsoverson - if you include the license that way can you still use Grunt tags in it?

For example:

Copyright (c) <%= grunt.template.today("yyyy") %> Doodle or Die, LLC.
Built on: <%= grunt.template.today("dddd, mmmm dS, yyyy, h:MM:ss TT") %>
Contributor

dylang commented Feb 8, 2013

Oh, I could easily create that license file in another task.

Hit this issue too, confusing as all get out until I disabled banners.
Maybe emit a warning when banner and sourceMaps are on, and
preserveComments : 'some'
or higher is not set?

jQuery ran afoul of this as reported at http://bugs.jquery.com/ticket/13776.

My thoughts: a fully general fix may not be possible, but (as a workaround while waiting for an upstream fix) you could avoid requiring preserveComments: "some" by putting a non-side-effect-free expression of equal size in place of the banner (e.g., /*!\nbanner\n*/++[\n000000\n];) and replacing it in the output.

karol-f commented Aug 28, 2013

Thank you for writing about this issue. It helped me a lot after hours of fiddling with code!

Another thank you for writing up this mystifying issue. This should really be in the docs!

Member

mgol commented Sep 27, 2013

For what it's worth, we're workarounding this issue in jQuery currently: https://github.com/jquery/jquery/blob/master/build/tasks/uglify.js

@azakus azakus referenced this issue in Polymer/polymer Oct 10, 2013

Closed

Debugging polymer apps: stack trace is wacko #307

azakus added a commit to Polymer/platform that referenced this issue Oct 10, 2013

@johnjbarton johnjbarton referenced this issue in ModuleLoader/es-module-loader Oct 25, 2013

Closed

Sourcemap is bonkers. #45

An alternative here would be to modify the source map to add in padding for the length of the banner lines. Luckily this is actually a straightforward operation:

Eg:

{
  mappings: "AABC,CCDD"
}

->

{
  mappings: ";;;;AABC,CCDD"
}

Where the banner is 4 lines long.

Could be an option if this isn't supported by uglify.

👍 to this bug. It reaaaaaally needs a fix. For the time being, I've moved banner to footer: "\n" + banner

@jsoverson jsoverson closed this in 03f1373 Oct 29, 2013

Member

jsoverson commented Oct 29, 2013

Added a warning. This task shouldn't have any extra logic to support this, though uglifyjs itself may find it useful to add.

Uglify supports keeping license comments in the source while maintaining sourcemap integrity, that just means adding the banner to a separate file and using that file as input to grunt-contrib-uglify. This seems like common practice, right? Having unminned files with the same banner and then uglifying that file?

Member

mgol commented Oct 29, 2013

@jsoverson Not necessarily. Many projects, like jQuery, use simpler banners for minified source; compare http://code.jquery.com/jquery-2.1.0-beta1.js and http://code.jquery.com/jquery-2.1.0-beta1.min.js.

Member

jsoverson commented Oct 29, 2013

Ok, makes sense. Opened up mishoo/UglifyJS2#335 to see if that could be brought in there. Without that, I may need to dive into Uglify and submit a PR (others very welcome)

@jsoverson jsoverson reopened this Oct 29, 2013

Member

jsoverson commented Nov 9, 2013

@mishoo added the preamble option for this case and we're mapping the 'banner' to it, so all forward usage should be OK with banners and sourcemaps.

Thanks to everyone involved.

@jsoverson jsoverson closed this Nov 9, 2013

mgol added a commit to jquery/jquery that referenced this issue Nov 9, 2013

mgol added a commit to jquery/jquery that referenced this issue Nov 9, 2013

Remove workarounds for the uglify task mishandling banners when used …
…with source maps. (cherry-picked from d0fadbb)

The issue was fixed in grunt-contrib-uglify:
gruntjs/grunt-contrib-uglify#22

azakus added a commit to Polymer/platform that referenced this issue Nov 13, 2013

azakus added a commit to Polymer/polymer that referenced this issue Nov 14, 2013

mescoda pushed a commit to mescoda/jquery that referenced this issue Nov 4, 2014

Remove workarounds for the uglify task mishandling banners when used …
…with source maps. (cherry-picked from d0fadbb)

The issue was fixed in grunt-contrib-uglify:
gruntjs/grunt-contrib-uglify#22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment