Fix Sourcemaps #515

schneems · 2017-11-13T22:36:25Z

Sorry for the large PR, these issues all cropped up at the same time.

Problem: Concat JS adds a newline after semi-colon

Solution: Don't add a newline. If one exists put semi-colon before newline

Problem: Directive Processor adds newline between assets

This was done after the "Default Sourcemap" processor so it wasn't encoded in the asset's "map"

Solution: Move the default sourcemap processor to be last

Problem: Default source map processor doesn't preserve source maps

Solution: Add the map back to the returned result

Problem: Assets with sourcemap comment do not end in newline

Solution: Add newline after sourcemap comment

(Not required, but it makes things nicer)

Problem: Concatenating Source Maps is broken

To concatenate 2 source maps you can use multiple "sections" and give each section an offset. To calculate the offset, you need to know the previous offset, and the length of the last generated file. The offset can be pulled from the last map. The size of the prior generated file was calculated using the number of semicolons in the "mappings"

sprockets/lib/sprockets/source_map_utils.rb

Line 80 in 341fed4

offset = a["sections"].last["map"]["mappings"].count(';') +

This behavior is not explicitly stated in the spec, but is implied and confirmed that it should work: https://groups.google.com/forum/#!topic/mozilla.dev.js-sourcemap/gp_ULp-h1fQ. However it appears that coffeescript generated sourcemaps do not always have the same number of semicolons as the generated files. I've opened an issue and with coffeescript jashkenas/coffeescript#4786

Solution: To get around this, I've added an x_sprockets_linecount key to the sourcemaps that includes the line count of the generated file. This key is added in the DefaultSourceMap which all assets will be processed through.

The downside of this approach is this now means that the concat source map function will only work on files that have been passed through the DefaultSourceMap processor.

The directive processor can add a newline to assets. If it comes after the default source map then this line is not accounted for when determining line offsets.

Adding a new line with semi-colons can cause off by 1 errors with source maps. Instead we want to add a semicolon. If the last character was whitespace such as a newline, we want to make sure that there are the same number of `str.lines` before and after, so we replace the whitespace character with a semicolon followed by the same whitespace character.

The calculation of length of asset is done by counting semicolons, however not all mappings end with a semicolon. When they do we must increment the offset. The next part is harder to explain. We concatenate two assets A.js and B.js. A has 10 lines B has 5 lines. After we’ve done this we want an offset that looks like this: ``` A.js: {"line"=>0, "column"=>0} B.js: {"line"=>11, "column"=>0} ``` To get 11 we take the prior length of mappings for A.js which is 10 (from the lines) and increment by one to get to 11. Now we add another asset C.js which has 2 lines. We want an offset that looks like this ``` A.js: {"line"=>0, "column"=>0} B.js: {"line"=>11, "column"=>0} C.js: {"line"=>16, "column"=>0} ``` If we add the last offset which is 11, to the length of the asset of B (which is 5) then we get the correct offset which is 16. We do not have to add an extra +1. I think this was accidentally working previously based on unwanted behavior from adding semi-colons and newlines.

This patch preserves an original source maps if one was present.

``` Warming up -------------------------------------- count 317.318k i/100ms length 329.512k i/100ms size 329.953k i/100ms Calculating ------------------------------------- count 9.806M (± 7.7%) i/s - 48.867M in 5.014627s length 11.696M (± 6.3%) i/s - 58.324M in 5.008040s size 11.074M (± 7.5%) i/s - 55.102M in 5.005626s ```

Based on the spec it is not guaranteed that there will be a mapping for each and every line https://groups.google.com/forum/#!topic/mozilla.dev.js-sourcemap/gp_ULp-h1fQ We were previously using this behavior to know the length of a previous asset when concatenating source maps. We can get around this by storing the length of the previous asset in an “x_sprockets_linecount” key. This is guaranteed to be on the asset since it is added by the `DefaultSourceMap`. We no longer need to offset the first asset by one line. This was accidentally working before.

We no longer add a newline if one did not previously exist.

travis-ci/travis-ci#8408 (comment)

Previous test was failing intermittently with results like `["0", "0", "1", "1", "0", "0", "1", “1”]`

We need this because `concat_source_maps` expects a `x_sprockets_linecount` key in the map metadata. It wasn’t put there until this PR, so if a map gets pulled out of the old cache and attempts to get loaded into memory then it will fail. By rev-ing the cache we can ensure the key will be there, since it’s added by `DefaultSourceMap`.

jeremy

Great batch of fixes ✌🏼

jeremy · 2017-11-14T21:14:08Z

.travis.yml

@@ -3,7 +3,7 @@ language: ruby
 cache: bundler

 sudo: false
-
+before_script: "unset _JAVA_OPTIONS"


Yep, causes a failure travis-ci/travis-ci#8408 (comment) and documentcloud/closure-compiler#48

jeremy · 2017-11-14T21:14:39Z

lib/sprockets.rb

+  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)


jeremy · 2017-11-14T21:14:53Z

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'


We need this because concat_source_maps expects a x_sprockets_linecount key in the map metadata. It wasn’t put there until this PR, so if a map gets pulled out of the old cache and attempts to get loaded into memory then it will fail.

By rev-ing the cache we can ensure the key will be there, since it’s added by DefaultSourceMap.

jeremy · 2017-11-14T21:18:36Z

lib/sprockets/utils.rb

@@ -100,18 +98,18 @@ def string_end_with_semicolon?(str)
    #
    # Returns buf String.
    def concat_javascript_sources(buf, source)
-      if source.bytesize > 0
-        buf << source
+      return buf if source.bytesize < 0


jeremy · 2017-11-14T21:22:58Z

lib/sprockets/utils.rb

-        elsif source[source.size - 1].ord != 0x0A
-          buf << "\n"
-        end
+      if whitespace = WHITESPACE_ORDINALS[buf[-1].ord]


Indexing into buf defeats the purpose of source encoding above?

That is a bug. I'm supposed to index into source thanks and good catch!

jeremy · 2017-11-14T21:25:51Z

lib/sprockets/utils.rb

+      if whitespace = WHITESPACE_ORDINALS[buf[-1].ord]
+        buf[-1] = ";#{whitespace}"
+      else
+        buf << ";"
      end


This is officially a "there be dragons" method. Your PR commentary would be appropriate for discussion in code comments here, too, so it's clear what conditions are at play and where they're implemented.

Switching from conditional expressions to conditional control flow makes this simpler to read in imperative style, but it's harder (for me) to parse and understand.

jeremy · 2017-11-14T21:27:31Z

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


Why are we asserting what this isn't instead of what it is?

When we're exporting things serially then the second exporter will not start until the first one ends. We are guaranteed to get 0 1 0 1 0 . However when we're running this in parallel, then we do not need to wait for the first exporter to be finished for the second to run, etc. That's why the original code asserted that we got four zeroes first.

However that test is coupled to a sleep and therefore race conditions.

I changed the test to allow for other combinations, as long as they don't perfectly match the "synchronous" case.

jeremy · 2017-11-14T21:27:38Z

test/test_manifest.rb

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


jeremy · 2017-11-14T21:27:50Z

test/test_source_map_utils.rb

+  def deep_dup(object)
+    Marshal.load( Marshal.dump(object) )
+  end
+


So we can take advantage of the UTF32 encoding.

schneems added 18 commits November 10, 2017 15:16

Default source map should come after Directive Processor

45f01e0

The directive processor can add a newline to assets. If it comes after the default source map then this line is not accounted for when determining line offsets.

Whitespace

870945b

Preserve original map if available

66c2bd7

This patch preserves an original source maps if one was present.

Only “reduce” source maps for css/js

ff5b6dd

Fix tests for concat_javascript_sources

30b5838

We no longer add a newline if one did not previously exist.

No newline after semicolon

3a0c297

Ensure asset ends with a newline

21fb47b

No newline after semicolon tests

9d808a4

Fix comment

b4f2dfa

Update tests for “x_sprockets_linecount”

8ad78ba

Unset _JAVA_OPTIONS

25d1fa8

travis-ci/travis-ci#8408 (comment)

Rescue when trying to dup an immutable object

809ecc9

Deep dup without refinements

b238342

More robust concurrent test

f2bbb41

Previous test was failing intermittently with results like `["0", "0", "1", "1", "0", "0", "1", “1”]`

schneems force-pushed the schneems/fix-newline-sourcemaps branch from c97c91d to f2bbb41 Compare November 14, 2017 17:31

schneems added 2 commits November 14, 2017 13:45

Test offsets

1c8756a

jeremy reviewed Nov 14, 2017

View reviewed changes

schneems added 3 commits November 14, 2017 16:15

Return if source bytesize is zero

e59e64d

Index into source instead of buf

3e75ea2

So we can take advantage of the UTF32 encoding.

Add comment explaining logic

b7f530b

schneems mentioned this pull request Nov 14, 2017

Concurrent asset compile #469

Merged

schneems merged commit 090ff0b into master Nov 14, 2017

This was referenced Nov 20, 2017

Fix sourcemap off lines #446

Closed

Don't add newlines when concatenating JS #419

Closed

jeremy deleted the schneems/fix-newline-sourcemaps branch March 12, 2018 23:10

chloerei mentioned this pull request Sep 13, 2021

Sprockets breaks the sourcemap comment rails/jsbundling-rails#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Sourcemaps #515

Fix Sourcemaps #515

schneems commented Nov 13, 2017 •

edited

Loading

jeremy left a comment

jeremy Nov 14, 2017

schneems Nov 14, 2017

jeremy Nov 14, 2017

jeremy Nov 14, 2017

schneems Nov 14, 2017

jeremy Nov 14, 2017

schneems Nov 14, 2017

jeremy Nov 14, 2017

schneems Nov 14, 2017

jeremy Nov 14, 2017

jeremy Nov 14, 2017

schneems Nov 14, 2017

jeremy Nov 14, 2017

jeremy Nov 14, 2017

schneems Nov 14, 2017

Fix Sourcemaps #515

Fix Sourcemaps #515

Conversation

schneems commented Nov 13, 2017 • edited Loading

Problem: Concat JS adds a newline after semi-colon

Problem: Directive Processor adds newline between assets

Problem: Default source map processor doesn't preserve source maps

Problem: Assets with sourcemap comment do not end in newline

Problem: Concatenating Source Maps is broken

jeremy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schneems commented Nov 13, 2017 •

edited

Loading