Merge pull request #515 from rails/schneems/fix-newline-sourcemaps

Fix Sourcemaps

.travis.yml

@@ -3,7 +3,7 @@ language: ruby
 cache: bundler
 
 sudo: false
-
+before_script: "unset _JAVA_OPTIONS"
 rvm:
   - 2.2
   - 2.3.4

lib/sprockets.rb

@@ -107,10 +107,6 @@ module Sprockets
     [SourceMapCommentProcessor]
   end
 
-  require 'sprockets/preprocessors/default_source_map'
-  register_preprocessor 'text/css', Preprocessors::DefaultSourceMap.new
-  register_preprocessor 'application/javascript', Preprocessors::DefaultSourceMap.new
-
   require 'sprockets/directive_processor'
   register_preprocessor 'text/css', DirectiveProcessor.new(comments: ["//", ["/*", "*/"]])
   register_preprocessor 'application/javascript', DirectiveProcessor.new(comments: ["//", ["/*", "*/"]])
@@ -123,7 +119,6 @@ module Sprockets
   register_bundle_metadata_reducer 'application/javascript', :data, proc { String.new("") }, Utils.method(:concat_javascript_sources)
   register_bundle_metadata_reducer '*/*', :links, :+
   register_bundle_metadata_reducer '*/*', :sources, proc { [] }, :+
-  register_bundle_metadata_reducer '*/*', :map, proc { |input| { "version" => 3, "file" => PathUtils.split_subpath(input[:load_path], input[:filename]), "sections" => [] } }, SourceMapUtils.method(:concat_source_maps)
 
   require 'sprockets/closure_compressor'
   require 'sprockets/sass_compressor'
@@ -219,4 +214,11 @@ module Sprockets
 
   depend_on 'environment-version'
   depend_on 'environment-paths'
+
+  require 'sprockets/preprocessors/default_source_map'
+  register_preprocessor 'text/css',               Preprocessors::DefaultSourceMap.new
+  register_preprocessor 'application/javascript', Preprocessors::DefaultSourceMap.new
+
+  register_bundle_metadata_reducer 'text/css',               :map, proc { |input| { "version" => 3, "file" => PathUtils.split_subpath(input[:load_path], input[:filename]), "sections" => [] } }, SourceMapUtils.method(:concat_source_maps)
+  register_bundle_metadata_reducer 'application/javascript', :map, proc { |input| { "version" => 3, "file" => PathUtils.split_subpath(input[:load_path], input[:filename]), "sections" => [] } }, SourceMapUtils.method(:concat_source_maps)
 end

lib/sprockets/cache.rb

@@ -51,7 +51,7 @@ class Cache
     # Internal: Cache key version for this class. Rarely should have to change
     # unless the cache format radically changes. Will be bump on major version
     # releases though.
-    VERSION = '4.0'
+    VERSION = '4.0.0'
 
     def self.default_logger
       logger = Logger.new($stderr)

lib/sprockets/preprocessors/default_source_map.rb

@@ -13,7 +13,7 @@ def call(input)
         map           = input[:metadata][:map]
         filename      = input[:filename]
         load_path     = input[:load_path]
-        lines         = input[:data].lines.count
+        lines         = input[:data].lines.length
         basename      = File.basename(filename)
         mime_exts     = input[:environment].config[:mime_exts]
         pipeline_exts = input[:environment].config[:pipeline_exts]
@@ -25,10 +25,14 @@ def call(input)
             "sources"   => [PathUtils.set_pipeline(basename, mime_exts, pipeline_exts, :source)],
             "names"     => []
           }
+        else
+          result[:map] = map
         end
+
+        result[:map]["x_sprockets_linecount"] = lines
         return result
       end
-      
+
       private
 
       def default_mappings(lines)

lib/sprockets/source_map_comment_processor.rb

@@ -29,7 +29,7 @@ def self.call(input)
       path = PathUtils.relative_path_from(PathUtils.split_subpath(input[:load_path], uri), map.digest_path)
 
       asset.metadata.merge(
-        data: asset.source + (comment % path),
+        data: asset.source + (comment % path) + "\n",
         links: asset.links + [asset.uri, map.uri]
       )
     end

lib/sprockets/source_map_utils.rb

@@ -48,7 +48,7 @@ def format_source_map(map, input)
         "sources"  => map["sources"].map do |source|
           source = URIUtils.split_file_uri(source)[2] if source.start_with? "file://"
           source = PathUtils.join(File.dirname(filename), source) unless PathUtils.absolute_path?(source)
-          _, source = PathUtils.paths_split(load_paths, source) 
+          _, source = PathUtils.paths_split(load_paths, source)
           source = PathUtils.relative_path_from(file, source)
           PathUtils.set_pipeline(source, mime_exts, pipeline_exts, :source)
         end,
@@ -72,13 +72,16 @@ def format_source_map(map, input)
     # Returns a new source map hash.
     def concat_source_maps(a, b)
       return a || b unless a && b
-      a, b = make_index_map(a), make_index_map(b)
+      a = make_index_map(a)
+      b = make_index_map(b)
 
-      if a["sections"].count == 0 || a["sections"].last["map"]["mappings"].empty?
-        offset = 0
-      else
-        offset = a["sections"].last["map"]["mappings"].count(';') + 
-                 a["sections"].last["offset"]["line"] + 1
+      offset = 0
+      if a["sections"].count != 0 && !a["sections"].last["map"]["mappings"].empty?
+        last_line_count = a["sections"].last["map"].delete("x_sprockets_linecount")
+        offset += last_line_count
+
+        last_offset = a["sections"].last["offset"]["line"]
+        offset += last_offset
       end
 
       a["sections"] += b["sections"].map do |section|

lib/sprockets/utils.rb

@@ -68,6 +68,9 @@ def hash_reassoc(hash, key_a, key_b = nil, &block)
       end
     end
 
+    WHITESPACE_ORDINALS = {0x0A => "\n", 0x20 => " ", 0x09 => "\t"}
+    private_constant :WHITESPACE_ORDINALS
+
     # Internal: Check if string has a trailing semicolon.
     #
     # str - String
@@ -79,14 +82,9 @@ def string_end_with_semicolon?(str)
         c = str[i].ord
         i -= 1
 
-        # Need to compare against the ordinals because the string can be UTF_8 or UTF_32LE encoded
-        # 0x0A == "\n"
-        # 0x20 == " "
-        # 0x09 == "\t"
-        # 0x3B == ";"
-        unless c == 0x0A || c == 0x20 || c == 0x09
-          return c === 0x3B
-        end
+        next if WHITESPACE_ORDINALS[c]
+
+        return c === 0x3B
       end
 
       true
@@ -100,18 +98,21 @@ def string_end_with_semicolon?(str)
     #
     # Returns buf String.
     def concat_javascript_sources(buf, source)
-      if source.bytesize > 0
-        buf << source
-
-        # If the source contains non-ASCII characters, indexing on it becomes O(N).
-        # This will lead to O(N^2) performance in string_end_with_semicolon?, so we should use 32 bit encoding to make sure indexing stays O(1)
-        source = source.encode(Encoding::UTF_32LE) unless source.ascii_only?
-
-        if !string_end_with_semicolon?(source)
-          buf << ";\n"
-        elsif source[source.size - 1].ord != 0x0A
-          buf << "\n"
-        end
+      return buf if source.bytesize <= 0
+
+      buf << source
+      # If the source contains non-ASCII characters, indexing on it becomes O(N).
+      # This will lead to O(N^2) performance in string_end_with_semicolon?, so we should use 32 bit encoding to make sure indexing stays O(1)
+      source = source.encode(Encoding::UTF_32LE) unless source.ascii_only?
+      return buf if string_end_with_semicolon?(source)
+
+      # If the last character in the string was whitespace,
+      # such as a newline, then we want to put the semicolon
+      # before the whitespace. Otherwise append a semicolon.
+      if whitespace = WHITESPACE_ORDINALS[source[-1].ord]
+        buf[-1] = ";#{whitespace}"
+      else
+        buf << ";"
       end
 
       buf

test/fixtures/source-maps/multi-require.js

@@ -0,0 +1,4 @@
+//= require child.js
+//= require coffee/main.js
+//= require sub/a.js
+//= require plain.js

test/fixtures/source-maps/plain.js

@@ -0,0 +1,3 @@
+function plain(input) {
+  console.log("Plain" + input)
+}

test/test_asset.rb

@@ -885,8 +885,7 @@ def setup
 
 define("application.js", "application-955b2dddd0d1449b1c617124b83b46300edadec06d561104f7f6165241b31a94.js")
 define("application.css", "application-46d50149c56fc370805f53c29f79b89a52d4cc479eeebcdc8db84ab116d7ab1a.css")
-define("POW.png", "POW-1da2e59df75d33d8b74c3d71feede698f203f136512cbaab20c68a5bdebd5800.png")
-;
+define("POW.png", "POW-1da2e59df75d33d8b74c3d71feede698f203f136512cbaab20c68a5bdebd5800.png");
     EOS
     assert_equal [
       "file://#{fixture_path_for_uri("asset/POW.png")}?type=image/png&id=xxx",
@@ -904,8 +903,7 @@ def setup
 
 define("application.js", "application-955b2dddd0d1449b1c617124b83b46300edadec06d561104f7f6165241b31a94.js")
 define("application.css", "application-46d50149c56fc370805f53c29f79b89a52d4cc479eeebcdc8db84ab116d7ab1a.css")
-define("POW.png", "POW-1da2e59df75d33d8b74c3d71feede698f203f136512cbaab20c68a5bdebd5800.png")
-;
+define("POW.png", "POW-1da2e59df75d33d8b74c3d71feede698f203f136512cbaab20c68a5bdebd5800.png");
     EOS
 
     assert_equal [
@@ -1080,13 +1078,22 @@ def setup
   end
 
   test "appends missing semicolons" do
-    assert_equal "var Bar\n;\n\n(function() {\n  var Foo\n})\n;\n",
-      asset("semicolons.js").to_s
+expected = <<-EOS
+var Bar;
+
+(function() {
+  var Foo
+});
+EOS
+    assert_equal expected, asset("semicolons.js").to_s
   end
 
   test 'keeps code in same line after multi-line comments' do
-    assert_equal "/******/ function foo() {\n}\n;\n",
-      asset('multi_line_comment.js').to_s
+expected = <<-EOS
+/******/ function foo() {
+};
+EOS
+    assert_equal expected, asset('multi_line_comment.js').to_s
   end
 
   test "should not fail if home is not set in environment" do
@@ -1134,7 +1141,7 @@ def setup
   end
 
   test "digest path" do
-    assert_equal "application.debug-60cfa762e3139c2580dbfbefd46a5359803225363eaef31efb725e6731ab6849.js",
+    assert_equal "application.debug-dee339ce0ee2dc7cdfa0d36dff3ef946cebe1bd3e414515d40c3cafc49c0a51a.js",
       @asset.digest_path
   end
 
@@ -1143,15 +1150,34 @@ def setup
   end
 
   test "length" do
-    assert_equal 264, @asset.length
+    assert_equal 265, @asset.length
   end
 
   test "charset is UTF-8" do
     assert_equal 'utf-8', @asset.charset
   end
 
   test "to_s" do
-    assert_equal "var Project = {\n  find: function(id) {\n  }\n};\nvar Users = {\n  find: function(id) {\n  }\n};\n\n\n\ndocument.on('dom:loaded', function() {\n  $('search').focus();\n});\n\n//# sourceMappingURL=application.js-161f7edec524c6e94ab3b89924c4fb96d4552bb83b32d975c074b7fb9a84c454.map", @asset.to_s
+expected = <<-EOS
+var Project = {
+  find: function(id) {
+  }
+};
+var Users = {
+  find: function(id) {
+  }
+};
+
+
+
+document.on('dom:loaded', function() {
+  $('search').focus();
+});
+
+//# sourceMappingURL=application.js-95e519d4e0f0a5c4c7d24787ded990b0d027f7ad30b39f402c4c5e3196a41e8b.map
+EOS
+
+    assert_equal expected, @asset.to_s
   end
 
   def asset(logical_path, options = {})

test/test_environment.rb

@@ -798,10 +798,12 @@ def foo; end
   end
 
   test "disabling default directive preprocessor" do
-    assert processor = @env.preprocessors['application/javascript'][0]
-    assert_kind_of Sprockets::DirectiveProcessor, processor
+    assert processors = @env.preprocessors['application/javascript']
+    processor = processors.detect {|p| p.is_a?(Sprockets::DirectiveProcessor) }
+
+    assert processor
     @env.unregister_preprocessor('application/javascript', processor)
-    assert_equal "// =require \"notfound\"\n;\n", @env["missing_require.js"].to_s
+    assert_equal "// =require \"notfound\";\n", @env["missing_require.js"].to_s
   end
 
   test "disabling processors by class name also disables processors which are instances of that class" do

test/test_manifest.rb

@@ -610,7 +610,7 @@ class SlowExporter2 < SlowExporter
     @env.register_exporter 'image/png',SlowExporter
     @env.register_exporter 'image/png',SlowExporter2
     Sprockets::Manifest.new(@env, @dir).compile('logo.png', 'troll.png')
-    assert_equal %w(0 0 0 0 1 1 1 1), SlowExporter.seq
+    refute_equal %w(0 1 0 1 0 1 0 1), SlowExporter.seq
   end
 
   test 'sequential exporting' do
@@ -642,7 +642,7 @@ def call(_)
     processor = SlowProcessor.new
     @env.register_postprocessor 'image/png', processor
     Sprockets::Manifest.new(@env, @dir).compile('logo.png', 'troll.png')
-    assert_equal %w(0 0 1 1), processor.seq
+    refute_equal %w(0 1 0 1), processor.seq
   end
 
   test 'sequential processing' do

test/test_source_map_utils.rb

@@ -4,6 +4,10 @@
 
 class TestSourceMapUtils < MiniTest::Test
 
+  def deep_dup(object)
+    Marshal.load( Marshal.dump(object) )
+  end
+
   def test_map
     source_map = {
       'version'  => 3,
@@ -94,12 +98,15 @@ def test_concat_empty_source_map_returns_original
       { source: 'c.js', generated: [rand(1..100), rand(0..100)], original: [rand(1..100), rand(0..100)] }
     ].freeze
 
+    linecount = abc_mappings.count(";")
+    linecount += 1 if abc_mappings[-1] == ";"
     map = {
       "version" => 3,
       "file" => "a.js",
       "mappings" => Sprockets::SourceMapUtils.encode_vlq_mappings(abc_mappings),
       "sources" => ["a.js", "b.js", "c.js"],
-      "names" => []
+      "names" => [],
+      "x_sprockets_linecount" => linecount
     }
     empty_map = { "version" => 3, "file" => 'a.js', "sections" => [] }
     index_map = {
@@ -113,11 +120,14 @@ def test_concat_empty_source_map_returns_original
       ]
     }
 
-    assert_equal map, Sprockets::SourceMapUtils.concat_source_maps(nil, map.dup)
-    assert_equal map, Sprockets::SourceMapUtils.concat_source_maps(map.dup, nil)
+    assert_equal map, Sprockets::SourceMapUtils.concat_source_maps(nil, deep_dup(map))
+    assert_equal map, Sprockets::SourceMapUtils.concat_source_maps(deep_dup(map), nil)
+
+    assert_equal index_map, Sprockets::SourceMapUtils.concat_source_maps(deep_dup(empty_map), map.dup)
 
-    assert_equal index_map, Sprockets::SourceMapUtils.concat_source_maps(empty_map.dup, map.dup)
-    assert_equal index_map, Sprockets::SourceMapUtils.concat_source_maps(map.dup, empty_map.dup)
+    expected_index_map = deep_dup(index_map)
+    expected_index_map["sections"].first["map"].delete("x_sprockets_linecount")
+    assert_equal expected_index_map, Sprockets::SourceMapUtils.concat_source_maps(deep_dup(map), deep_dup(empty_map))
   end
 
   def test_concat_source_maps
@@ -136,27 +146,29 @@ def test_concat_source_maps
       "file" => "a.js",
       "mappings" => Sprockets::SourceMapUtils.encode_vlq_mappings(abc_mappings),
       "sources" => ["a.js", "b.js", "c.js"],
-      "names" => []
+      "names" => [],
+      "x_sprockets_linecount" => 3
+
     }
 
     d_map = {
       "version" => 3,
       "file" => "d.js",
       "mappings" => Sprockets::SourceMapUtils.encode_vlq_mappings(d_mapping),
       "sources" => ["d.js"],
-      "names" => []
+      "names" => [],
+      "x_sprockets_linecount" => 1
     }
 
-
-    combined = Sprockets::SourceMapUtils.concat_source_maps(abc_map.dup, d_map.dup)
+    combined = Sprockets::SourceMapUtils.concat_source_maps(deep_dup(abc_map), deep_dup(d_map))
     assert_equal [
       { source: 'a.js', generated: [1, 0], original: [1,  0] },
       { source: 'b.js', generated: [2, 0], original: [20, 0] },
       { source: 'c.js', generated: [3, 0], original: [30, 0] },
       { source: 'd.js', generated: [4, 0], original: [1,  0] }
     ], Sprockets::SourceMapUtils.decode_source_map(combined)[:mappings]
 
-    combined = Sprockets::SourceMapUtils.concat_source_maps(d_map.dup, abc_map.dup)
+    combined = Sprockets::SourceMapUtils.concat_source_maps(deep_dup(d_map), deep_dup(abc_map))
     assert_equal [
       { source: 'd.js', generated: [1, 0], original: [1,  0] },
       { source: 'a.js', generated: [2, 0], original: [1,  0] },