Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fix #13274: Wrap sourceMap directive in multiline comments. Close gh-…

…1143.

(cherry picked from commit ac93559)
  • Loading branch information...
commit 487b703521e63188102c73e8ce6ce203d28f260b 1 parent fb0f295
@gibson042 gibson042 authored
Showing with 42 additions and 2 deletions.
  1. +13 −2 Gruntfile.js
  2. +22 −0 test/data/core/cc_on.html
  3. +7 −0 test/unit/core.js
View
15 Gruntfile.js
@@ -409,7 +409,7 @@ module.exports = function( grunt ) {
nonascii = false;
distpaths.forEach(function( filename ) {
- var i, c,
+ var i, c, map,
text = fs.readFileSync( filename, "utf8" );
// Ensure files use only \n for line endings, not \r\n
@@ -438,7 +438,18 @@ module.exports = function( grunt ) {
text = text.replace( /"dist\//g, "\"" );
fs.writeFileSync( filename, text, "utf-8" );
} else if ( /\.min\.js$/.test( filename ) ) {
- text = text.replace( /sourceMappingURL=dist\//, "sourceMappingURL=" );
+ // Wrap sourceMap directive in multiline comments (#13274)
@jdalton
jdalton added a note

Should this be handled in Grunt? /cc @cowboy

@dmethvin Owner

Uglify2 just got a --source-map-url option to fix the problem with the dist/ appearing in the url there, but it's only half fixed. I think the wrapping in a comment should be handled inside Uglify2 as well since it affects everyone and not just grunt or jQuery users. I opened ticket 108 there. When that gets fixed we'll need to remove our wrap lest we get double-wrapped.

@jdalton
jdalton added a note

@dmethvin Ah, thanks for that!

@gibson042 Collaborator

Thanks @dmethvin. We should be safe from double wrapping, but will end up with an empty comment at the end of the file after we use a version of Uglify2 that fixes this (since I move the directive into our existing banner).

@cowboy
cowboy added a note

Feel free to open an issue on https://github.com/gruntjs/grunt-contrib-uglify :+1:

@gibson042 Collaborator

Actually, the more I think about this, the more I like the idea of managing the source map directive ourselves. Why have two comments in the minified source?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ text = text.replace( /\n?(\/\/@\s*sourceMappingURL=)(.*)/,
+ function( _, directive, path ) {
+ map = "\n" + directive + path.replace( /^dist\//, "" );
+ return "";
+ });
+ if ( map ) {
+ text = text.replace( /(^\/\*[\w\W]*?)\s*\*\/|$/,
+ function( _, comment ) {
+ return ( comment || "\n/*" ) + map + "\n*/";
+ });
+ }
fs.writeFileSync( filename, text, "utf-8" );
}
View
22 test/data/core/cc_on.html
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html lang="en">
+<head>
+ <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
+ <script>
+ var cc_on = false,
+ errors = [];
+/*@cc_on
+ cc_on = true;
+@*/
+ window.onerror = function( errorMessage, filePath, lineNumber ) {
+ errors.push( errorMessage );
+ };
+ </script>
+ <script src="../../../dist/jquery.min.js"></script>
+</head>
+<body>
+ <script>
+ window.parent.iframeCallback( cc_on, errors, jQuery );
+ </script>
+</body>
+</html>
View
7 test/unit/core.js
@@ -17,6 +17,13 @@ test("Basic requirements", function() {
ok( $, "$" );
});
+testIframeWithCallback( "Conditional compilation compatibility (#13274)", "core/cc_on.html", function( cc_on, errors, $ ) {
+ expect( 3 );
+ ok( true, "JScript conditional compilation " + ( cc_on ? "supported" : "not supported" ) );
+ deepEqual( errors, [], "No errors" );
+ ok( $(), "jQuery executes" );
+});
+
test("jQuery()", function() {
var elem, i,
@jdalton

Should this be handled in Grunt? /cc @cowboy

@dmethvin

Uglify2 just got a --source-map-url option to fix the problem with the dist/ appearing in the url there, but it's only half fixed. I think the wrapping in a comment should be handled inside Uglify2 as well since it affects everyone and not just grunt or jQuery users. I opened ticket 108 there. When that gets fixed we'll need to remove our wrap lest we get double-wrapped.

@jdalton

@dmethvin Ah, thanks for that!

@gibson042
Collaborator

Thanks @dmethvin. We should be safe from double wrapping, but will end up with an empty comment at the end of the file after we use a version of Uglify2 that fixes this (since I move the directive into our existing banner).

@gibson042
Collaborator

Actually, the more I think about this, the more I like the idea of managing the source map directive ourselves. Why have two comments in the minified source?

Please sign in to comment.
Something went wrong with that request. Please try again.