Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only compile source maps in the debug pipeline #410

Open
TylerHorth opened this issue Oct 25, 2016 · 17 comments
Open

Only compile source maps in the debug pipeline #410

TylerHorth opened this issue Oct 25, 2016 · 17 comments

Comments

@TylerHorth
Copy link

TylerHorth commented Oct 25, 2016

Currently in sprockets 4 source maps are compiled in the default pipeline alongside everything else, then source map comments are appended in the debug pipeline.

This causes a massive performance degradation when compiling assets, even when source maps aren't being used. Shopify saw compilation time go from 1m10s to 7m20s when upgrading to sprockets 4. StrippingHacking out source map logic from sprockets 4 returned compilation time to 60s.

cc @bouk @rafaelfranca

@vincentwoo
Copy link

Ah, well, I think a lot of us will want source maps in prod for debugging. I suppose the question becomes: opt-in or opt-out? Probably decent arguments to be made either way.

@TylerHorth
Copy link
Author

@vincentwoo Could you explain your use case in a bit more detail? How are you using source maps without source map comments? Are you uploading them to a bug tracker?

@vincentwoo
Copy link

@gilesbowkett
Copy link

this might be a duplicate of #413. it's definitely related.

@PikachuEXE
Copy link

I want source map in prod too (for error tracking, same as @vincentwoo)
Anything we can do to achieve that quicker?

@mjc-gh
Copy link

mjc-gh commented Jan 3, 2017

Our team is also looking into generating source maps in production/staging for error reporting (via Airbrake).

For our particular case, we need to explicitly configure our error reporter with the source map locations and do not need to include source map URL comments in the minified results.

@schneems
Copy link
Member

schneems commented Jan 4, 2017

im looking into some architectural changes that can hopefully make generating source maps optional, or rather only do them when requesting a debug asset.

Progress is slow though. I want to change how assets are loaded, the current implementation of "pipelines" is challenging to work with.

@mjc-gh
Copy link

mjc-gh commented Jan 4, 2017

As far as source maps in dev with debug mode enabled goes, everything is working fine off of master (this is with a Rails v5.0.1 prject).

For production and staging we wanted to generate source maps without including the comment in the actual minified JavaScript files. Instead, we need to just tell our error reporter (Airbrake) about the map files.

To achieve this, we created a small Compressor which wraps the Uglifier compressor and links the source maps so they are written to disk.

class UglifyWithSourceMaps < Sprockets::UglifierCompressor
  def call(input)
    result = super(input)

    map_uri = input[:environment].build_asset_uri(input[:filename], {
      type: 'application/js-sourcemap+json'
    })

    links = Set.new(input[:metadata][:links])
    links << map_uri

    result.update links: links
  end
end

Sprockets.register_compressor 'application/javascript', :uglify_with_source_maps, UglifyWithSourceMaps

In our production and staging environments, we now use this compressor.

config.assets.js_compressor = :uglify_with_source_maps

We then added another small JavaScript file to let our error handler know about these map files:

airbrake.addFilter(function(notice) {
  notice.context.sourceMaps = {
    '<%= asset_url 'editor.js' %>': '<%= asset_url 'editor.js.map' %>'
  }
});

@schneems
Copy link
Member

schneems commented Jan 4, 2017

Not sure here, but I think you should be able to do that directly inside of your JS file

//= link editor.js.map

I think that's how I would want to support the use case of generating a map file in staging/production, but i'm not 100% on that. What do you think?

@vincentwoo
Copy link

I think that's fine, but I also don't particularly care about @mikeycgto's desire to not have the linking comment be present in the minified JS.

@mjc-gh
Copy link

mjc-gh commented Jan 4, 2017

Thanks for the advice @schneems. Using the link directive works and is a lot cleaner than using a custom compressor.

We're indifferent to the source map comment being present in minified files. It's just for our particular use case we just need explicitly tell our error reporter about the source maps and, more importantly, have a public URL to provide to the reporter.

@schneems
Copy link
Member

schneems commented Jan 4, 2017

Glad that worked. Now I just need to figure out how to decouple this ball of mud. One asset can generate up to four seperate files. The way we do it currently is by calling load from inside of load, via different "pipelines" and processors which is quite elegant and completely impossible to work with.

Here's the four case:

  • foo.js
    • Load/Require dependencies
    • Concatenate dependencies
  • foo.js.map
    • Load foo.js
    • Currently grabs metadata[:map] from asset to build an asset, need to move that generation somewhere else to accomplish de-coupling map generation
  • foo.debug.js
    • Load foo.js
    • Load foo.js.map
    • Add comment to end of foo.js with path to foo.js.map
  • foo.source.js
    • The raw file on disk, the map file will need to point to source files.

I'm kinda stuck at the moment, going around in circles. Everything is really heavily coupled. I would like to get to the point where no load is called from within processors, but i'm not sure if that's possible. Currently the API and the caching strategies are fighting me at every step of the way. I have a branch where i'm hacking through some refactoring, no light at the end of the tunnel yet though :(

@mjc-gh
Copy link

mjc-gh commented Jan 4, 2017

Certainly appreciate the work you've done! If I can find some time, maybe I can help pitch in somehow.

@vincentwoo
Copy link

Yeah, can we pay money to make this go faster? Serious question.

@gilesbowkett
Copy link

@vincentwoo I don't know re 💰 but based on @schneems saying

Now I just need to figure out how to decouple this ball of mud.

my guess is that automated tests and written documentation would both help. 😃

@schneems
Copy link
Member

schneems commented Jan 5, 2017

For the $$$ question, nothing comes to mind. These problems i'm hitting up against are larger than a contractor could solve in a few hours of work (which would be hundreds/thousands of dollars). Right now major changes require a deep and broad understanding of the codebase and how things get done. If a company really wanted to invest, I would prefer they dedicated an employee for X hours a week for Y months than money.

Tyler and Bouk have both been very helpful in pushing the project forwards.

Giles is also right, docs and tests are good. Running the betas and reporting problems is good. Triaging issues, reproducing bugs, fixing reported bugs are all helpful. Money could be good if it is spent to provide some of the above things. Money on it's own is hard because then it means I would have to spend time book-keeping and managing instead of programming.

@schneems
Copy link
Member

The way that source maps work is they're generated from the processor. For example the coffee script processor calls coffeescript which returns a source map.

Some of these tools have the ability to turn off source maps. What I think we need to do is introduce a new config just for source maps that is auto enabled when debug is on. When we process assets we can use the new config to tell sources not to run build source maps. Then we tell sprockets to not use the DefaultSourceMap processor.

That should work, but it's a non-trivial change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants