Compress source maps #402

TylerHorth · 2016-10-13T20:17:20Z

Currently the source map processor expects sources to be in the format of a logical path, however the source map spec only supports source maps with absolute paths or relative paths. I introduced a workaround in #390 which changed logical paths to relative paths. But this solution is extremely fragile.

This PR introduces a proper fix. The source map processor now expects either relative paths, or absolute paths, and takes care of the necessary processing such that they are resolved properly. The overall effect of this is that individual processors no longer need to worry about producing sprockets compatible maps, that logic is handled by the source map processor.

This PR also removes fingerprinting from source files as reported in #343. Since the source map processor includes fingerprinted sources in the asset's metadata, it is not necessary to also include them in the data. I modified the test introduced in #367 to reflect this change.

rails-bot · 2016-10-13T20:17:22Z

r? @arthurnn

(@rails-bot has picked a reviewer for you, use r? to override)

TylerHorth · 2016-10-17T18:50:07Z

I should add, logical paths would be acceptable if the sourceRoot attribute is present in the map, and sourceRoot + source is an absolute path to the asset. However, only the mappings attribute is being passed to the SourceMapProcessor so there is no way to check for this attribute, and some processors don't support the sourceRoot property, so support for relative/absolute paths is necessary regardless.
It is possible to support this third case if the entire map is returned in the asset metadata instead of just map['mappings'], but there wouldn't be much value added.

schneems · 2016-10-17T18:54:27Z

test/test_asset.rb

@@ -1106,7 +1106,7 @@ def setup
  end

  test "digest path" do
-    assert_equal "application.debug-2f5fde4066077205c961164247ca6ae471977d347b4ef96d9d6e2e17d9d9906c.js",
+    assert_equal "application.debug-3fbae0574ecaa976f947b4dabdaa5975cac5011ee5196cfb5b8e464c7921314c.js",


why are the shas changing here? I get nervous when these change in tests.

This digest is for an asset compiled in debug mode, which means it has a sourceMappingURL comment which links to a source map. I think this assets digest has changed because the digest of it's source map has changed: the sources no longer contain fingerprints in their names.

TylerHorth · 2016-10-17T21:42:23Z

This PR is introducing some errors for me, but I haven't been able to narrow down what's causing it.

Source maps aren't working with index files (i.e. #= link foo/baz.js where baz is a folder with file foo/baz/index.js). However this was the case before this PR as well. I think these two problems may be related; but before it was just failing silently, whereas now we attempt to resolve the nonexistent file.

Consider this PR on hold until I figure out exactly what's going wrong.

TylerHorth · 2016-10-20T15:39:40Z

I haven't yet been able to isolate the problem I was experiencing earlier, but I discovered a new one.

I made a mistake before by saying fingerprints were included in the links metadata, they're not. My test was only passing since it was broken. I've fixed the test now and verified that it is failing.

Removing fingerprints from sources wasn't the main point of this PR but I certainly believe it should be done. I can probably fix this by simply adding another metadata attribute, but I'm going to see if there's a cleaner way to do it.

Edit: So uh, all is well again. Test was failing because the source maps weren't any different. The source files are indeed in the dependencies already so no extra work is necessary. This ordeal has been one big lesson in writing good tests.

schneems · 2016-10-21T15:12:37Z

Where are you at here? Is this good to go? Thanks for all the work!

TylerHorth · 2016-10-21T16:43:51Z

This PR introduces issues for me in a large project I tested it in. I haven't been able to narrow down the source yet, just want to make sure it isn't a problem with this PR before shipping it.

I'll report back here, probably today, when I figure out what is going on.

TylerHorth · 2016-10-24T17:25:34Z

Alright, the core of the problem seems to be with concat_source_maps. When the two maps are not from the same directory, it should extend each source if they are relative. This problem is demonstrated in the test case I added.

sub
├── a.js
├── directory.js
└── modules
    └── something.js

//# sub/directory.js
//= require ./a
//= require ./modules/something

  1) Failure:
TestSourceMaps#test_"relative sources at different depths" [C:/projects/sprockets/test/test_source_maps.rb:253]:
--- expected
+++ actual
@@ -1 +1 @@
-["a.source.js", "modules/something.source.js", "directory.source.js"]
+["a.source.js", "something.source.js", "directory.source.js"]

I also foresee a larger problem to do with the mapping between sources and sprockets virtual file system. Paths outside the /assets folder can be effectively "mounted" to within by appending an asset path. This will however break the current logic of making relative paths as they will be relative to the physical location rather than the virtual one, and thus requests for them will land outside of the /assets scope. I imagine this can be resolved by generating relative paths using the logical paths of each asset rather than the absolute paths.

I'm going to have to be more cognizant of the virtual filesystem in the future.

TylerHorth · 2016-11-04T16:05:35Z

Okay, this PR's turned into a much bigger monster than originally intended. It changes a lot of stuff, here's the breakdown:

Most significantly, it changes the format of the :map metadata attribute from an array of mappings to source map hash compliant with the source map v3 spec.
- This is a breaking change. Any processor which produces source maps will need to be updated to support it. I've updated all internal sprockets processors in this PR.
- As the source map filename is included alongside the sources in the map, it is possible for the source map processor to format the sources so they are resolved correctly.
- Much faster. Source maps according to spec use a compressed format which is much smaller. Compile time on Shopify went from 7m10s to 3m50s.
Concatenates files by producing an index map.
- The source map spec also defines a spec for an index map. An index map is a special form of map with the express purpose of simplifying the task of joining maps.
- Allows us to concatenate source maps without decoding them.
SourceMapProcessor formats sources
- The extra data included in the map metadata makes it possible to correctly resolve sources (if I've got the logic correct).
- No longer adds fingerprints to sources as they are already included in the dependencies list.

Problems:

Tests are failing. I've got no idea why, I'm unable to reproduce locally.
Combining source maps requires decoding them. This is slow, but it's miles faster than storing them in their decoded form. The problem is that my current decode_source_map implementation merges all sections, obliterating section file names. This will cause sources to be resolved incorrectly as they are relative to their sections file name, not the maps file name which is preserved. I need to find a way to combine index source maps without merging the sections. edit: It's probably ok to just leave this the way it is, possibly throw an exception if ever someone tries to combine index maps. Should never happen.

schneems · 2016-11-04T16:07:13Z

There is a lot going on here, I honestly haven't been following too intently. It will be some time before I can really dig in. Are you going to be at RubyConf?

TylerHorth · 2016-11-04T16:57:35Z

Are you going to be at RubyConf?

Unfortunately, I will not.

Take your time, there's still some work to be done here anyway.

TylerHorth · 2016-11-09T23:35:56Z

This PR is brokend atm. Some weird shenanigans are happening with caching, causing source uris to get malformed after a file is changed and recompiled.

Steps to reproduce:

Clear cache. rm -rf tmp/cache
Load page (with debug pipeline).
Modify any file.
Reload page.
=>

Sprockets::FileNotFound in Root#index
couldn't find file 'sub/modules/modules/something.source.js'
Checked in these paths:
/Users/tylerhorth/src/github.com/TylerHorth/sprockets/test/dummy/app/assets/config
/Users/tylerhorth/src/github.com/TylerHorth/sprockets/test/dummy/app/assets/images
/Users/tylerhorth/src/github.com/TylerHorth/sprockets/test/dummy/app/assets/javascripts
/Users/tylerhorth/src/github.com/TylerHorth/sprockets/test/dummy/app/assets/stylesheets
/Users/tylerhorth/src/github.com/TylerHorth/sprockets/test/dummy/vendor/assets/javascripts
/Users/tylerhorth/src/github.com/TylerHorth/sprockets/test/dummy/vendor/assets/stylesheets
/Users/tylerhorth/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/jquery-rails-4.2.1/vendor/assets/javascripts
/Users/tylerhorth/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/coffee-rails-4.1.1/lib/assets/javascripts
/Users/tylerhorth/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/actioncable-5.0.0.1/lib/assets/compiled
/Users/tylerhorth/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/turbolinks-source-5.0.0/lib/assets/javascripts
/Users/tylerhorth/src/github.com/TylerHorth/sprockets/test/fixtures

TylerHorth · 2016-11-11T16:53:25Z

There's two separate problems here.

Source maps can contain absolute paths. This is what was breaking CI. When asset metadata contains absolute paths, sprockets compresses them before saving them to cache so they don't affect fingerprints. To accommodate this, map attributes either need to always be absolute (so they can be compressed and decompressed), or always be logical. This would need to be done by the processors which generate the maps.
When a file is changed, source maps are re-processed, even those that didn't change. This is because the source map processor is executed after assets are bundled. So some of the map sections have been processed, and some have not. This would be ok if processed and unprocessed maps were both valid which was the original intention of this PR, but presently they get invalidated by the concat_source_map step as the relative sources are not rerooted to the new map file. This part's not completely correct. They were getting reprocessed, just not for this reason.

So basically, the solution is to do exactly the opposite of the title of this PR: move map formatting logic out of the source map processor. Each processor should call a method to format its generated map, and concat_source_maps should reroot sources after the maps have been combined. This should ensure that at every step of the process maps are in a standard, valid format, leaving no room for things to get screwed up along the way. Hopefully.

- :map metadata attribute complies with the source map v3 spec - Supports index maps - Reduced memory overhead - Improved performance

TylerHorth · 2016-11-12T00:10:35Z

I've fixed the tests and rebased.

What I've done is created a format_source_map method which each processor calls before returning its data. This method transforms source maps into a standard format: the file attribute is a logical path, and the sources are all made relative paths.

When source maps are concatenated, concat_source_maps handles rerooting the sources. This ensures that through every step of the process, the source map is in a valid state.

No additional work is required when the source maps are saved to cache, as there are no absolute paths.

I've also renamed this PR since the source map processor isn't actually doing any formatting of sources.

I think this PR is finally finished...

schneems · 2016-11-15T16:17:15Z

Thanks for all your work!

schneems · 2016-12-12T16:37:50Z

Merged! Thanks again.

jtomaszewski · 2017-03-20T16:42:09Z

How do you make it run guys?

I get a lot of weird errors when trying to run it with current master (it worked previously for beta3):

f.e. during javascript_include_tag 'application'

Sprockets::ConversionError  could not convert nil to "application/javascript"

/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:133:in `load_from_unloaded'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:59:in `block in load'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:318:in `fetch_asset_from_dependency_cache'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:43:in `load'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/cached_environment.rb:44:in `load'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/bundle.rb:41:in `block in call'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/utils.rb:166:in `dfs'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/bundle.rb:42:in `call'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/processor_utils.rb:84:in `call_processor'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/processor_utils.rb:66:in `block in call_processors'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/processor_utils.rb:65:in `reverse_each'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/processor_utils.rb:65:in `call_processors'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:144:in `load_from_unloaded'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:59:in `block in load'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:318:in `fetch_asset_from_dependency_cache'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:43:in `load'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/cached_environment.rb:44:in `load'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/source_map_comment_processor.rb:22:in `call'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/processor_utils.rb:84:in `call_processor'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/processor_utils.rb:66:in `block in call_processors'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/processor_utils.rb:65:in `reverse_each'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/processor_utils.rb:65:in `call_processors'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:144:in `load_from_unloaded'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:59:in `block in load'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:318:in `fetch_asset_from_dependency_cache'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/loader.rb:43:in `load'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/cached_environment.rb:44:in `load'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/base.rb:69:in `find_asset'
/opt/boxen/rbenv/versions/2.3.3/lib/ruby/gems/2.3.0/bundler/gems/sprockets-09f44cb0334b/lib/sprockets/base.rb:95:in `[]'
sprockets-rails (3.2.0) lib/sprockets/rails/helper.rb:355:in `find_asset'
sprockets-rails (3.2.0) lib/sprockets/rails/helper.rb:347:in `find_debug_asset'
sprockets-rails (3.2.0) lib/sprockets/rails/helper.rb:229:in `block in lookup_debug_asset'

schneems · 2017-03-20T17:29:09Z

@jtomaszewski Please open up a new issue and attach an example app.

rails-bot assigned arthurnn Oct 13, 2016

schneems reviewed Oct 17, 2016

View reviewed changes

TylerHorth mentioned this pull request Oct 31, 2016

Sprockets 4 performance regressions #413

Closed

TylerHorth force-pushed the source-map-processor-formats-sources branch 5 times, most recently from 0c244ea to ce24af1 Compare November 4, 2016 15:30

TylerHorth mentioned this pull request Nov 4, 2016

Add config option for memory cache size #409

Closed

TylerHorth force-pushed the source-map-processor-formats-sources branch from 66930cd to 328b23a Compare November 11, 2016 23:44

Compress source maps

386ab2b

- :map metadata attribute complies with the source map v3 spec - Supports index maps - Reduced memory overhead - Improved performance

TylerHorth force-pushed the source-map-processor-formats-sources branch from 328b23a to 386ab2b Compare November 11, 2016 23:58

TylerHorth changed the title ~~SourceMapProcessor formats sources~~ Compress source maps Nov 11, 2016

schneems merged commit 386ab2b into rails:master Dec 12, 2016

y-yagi mentioned this pull request Sep 3, 2017

NoMethodError: undefined method `key?' for #<Array:0x007fdbee0dfbc0> #492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compress source maps #402

Compress source maps #402

TylerHorth commented Oct 13, 2016

rails-bot commented Oct 13, 2016

TylerHorth commented Oct 17, 2016 •

edited

schneems Oct 17, 2016

TylerHorth Oct 17, 2016 •

edited

TylerHorth commented Oct 17, 2016 •

edited

TylerHorth commented Oct 20, 2016 •

edited

schneems commented Oct 21, 2016

TylerHorth commented Oct 21, 2016

TylerHorth commented Oct 24, 2016 •

edited

TylerHorth commented Nov 4, 2016 •

edited

schneems commented Nov 4, 2016

TylerHorth commented Nov 4, 2016 •

edited

TylerHorth commented Nov 9, 2016 •

edited

TylerHorth commented Nov 11, 2016 •

edited

TylerHorth commented Nov 12, 2016 •

edited

schneems commented Nov 15, 2016

schneems commented Dec 12, 2016

jtomaszewski commented Mar 20, 2017

schneems commented Mar 20, 2017

Compress source maps #402

Compress source maps #402

Conversation

TylerHorth commented Oct 13, 2016

rails-bot commented Oct 13, 2016

TylerHorth commented Oct 17, 2016 • edited

schneems Oct 17, 2016

Choose a reason for hiding this comment

TylerHorth Oct 17, 2016 • edited

Choose a reason for hiding this comment

TylerHorth commented Oct 17, 2016 • edited

TylerHorth commented Oct 20, 2016 • edited

schneems commented Oct 21, 2016

TylerHorth commented Oct 21, 2016

TylerHorth commented Oct 24, 2016 • edited

TylerHorth commented Nov 4, 2016 • edited

schneems commented Nov 4, 2016

TylerHorth commented Nov 4, 2016 • edited

TylerHorth commented Nov 9, 2016 • edited

TylerHorth commented Nov 11, 2016 • edited

TylerHorth commented Nov 12, 2016 • edited

schneems commented Nov 15, 2016

schneems commented Dec 12, 2016

jtomaszewski commented Mar 20, 2017

schneems commented Mar 20, 2017

TylerHorth commented Oct 17, 2016 •

edited

TylerHorth Oct 17, 2016 •

edited

TylerHorth commented Oct 17, 2016 •

edited

TylerHorth commented Oct 20, 2016 •

edited

TylerHorth commented Oct 24, 2016 •

edited

TylerHorth commented Nov 4, 2016 •

edited

TylerHorth commented Nov 4, 2016 •

edited

TylerHorth commented Nov 9, 2016 •

edited

TylerHorth commented Nov 11, 2016 •

edited

TylerHorth commented Nov 12, 2016 •

edited