Skip to content
This repository has been archived by the owner on Jan 16, 2022. It is now read-only.

Add support for sprockets 4 and source maps #296

Merged
merged 2 commits into from
Feb 27, 2018

Conversation

mgodwin
Copy link
Contributor

@mgodwin mgodwin commented Feb 3, 2018

This adds support for Sprockets 4 and subsequently source maps.

Motivation

On large Rails JS codebases, the current strategy of loading each js file from the asset pipeline separately can be very time consuming. On a project I'm working on with around ~1400 js files, the time to load the page before specs even begin executing is ~10-15s. This was echoed by #240 and PR #241. (It's also an issue that other jasmine/rails implementations struggle with testdouble/jasmine-rails#148.)

Solution

Rails has long had the config.assets.debug = false option, which causes your asset files to be concatenated in development, to workaround making n requests for each asset in the assets/ folder. My initial approach was to introduce a similar configuration option into jasmine-gem, but I discovered quickly that you lose the usefulness of your stack trace when you concatenate your assets together for spec tests. (The page does load substantially faster though!)

After doing some research, I learned that Sprockets 4 was taking a different approach to asset concatenation - essentially removing the config.assets.debug option, because it always concatenates assets. Sprockets 4 additionally uses source maps to get the same individual file fidelity as the debug option, without incurring the cost of loading each file individually.

So, this PR introduces support for Sprockets 4, which supports source maps.

(Side Note: I'd still like to open another PR to add a config option that allows toggling of asset concatenation to provide those running older versions of rails a way to more speedily run their tests.)

Details

I somewhat heavily modified the code in asset_expander.rb to more closely reflect what sprockets-rails does to resolve asset paths in the javascript_include_tag and stylesheet_include_tag helpers. This seemed like a good decision for a couple of reasons:

  1. Since sprockets-rails already has support for Sprockets 4, this made it easy to hook into the helpers they provide for serving up source mapped assets.
  2. This way we're not reinventing the wheel when it comes to resolving assets because we can easily leverage the somewhat complex asset resolution code provided by sprockets-rails.

The sprockets-rails code also nicely degrades for older versions of sprockets, checking whether the asset responds_to? :to_a as a mechanism for determining sprockets version. (:to_a was removed in sprockets 4.)

How it works

I've taken quite a few screenshots to illustrate how this all works. I'd be up for writing some tests to validate this behavior, but I'm not 100% clear on the best way to branch the testing code (more on that later).

Here's what our assets look like before this PR is introduced while using Sprockets 4:
sprockets-4-without-pr

And after this PR is introduced - you'll can see the source files are notated as .source.js:
sprockets-4-with-pr-source-maps

One downside I discovered right after doing this was that the native Error.stack that jasmine uses doesn't report locations with source maps, so you get just the concatenated asset:
sprockets-4-stacktrace

As it turns out, there's an issue to track this on the core jasmine repo jasmine/jasmine#491 but you can work around that limitation by installing a helper_script that monkey patches Error.stack to look at source maps if present. (One project that can help you do that is node-source-map-support). After monkey patching, you get something like this 馃檶 :

sprockets4-stacktrace-sourcemaps

(FWIW - I wouldn't normally advocate for monkey patching, but it does seem like the native Error.stack should be robust enough to support source maps. I don't know if it should be Jasmine's responsibility to take care of it as an ExceptionFormatter - but either way, this absolutely works until a more permanent solution is found.)

Testing

You can test this out by checking out the gem and creating a new rails project. Modify the Gemfile to reference your local version of jasmine-gem and add gem 'sprockets', github: 'rails/sprockets' to get Sprockets 4. After initializing jasmine in the repo, you should be able to run rake jasmine and see the source mapped files!

Other changes

There were a couple of extra things I went ahead and fixed while I was in asset_expander.rb or the related spec.

Double appended body query string param
You have to append the ?body=true query string parameter to get sprockets to serve the contents of the individual asset file, but starting with rails 4, this was always automatically appended when requesting an asset with the debug: true option. So all assets had the query string param appended twice:

double-body-query-string

In my testing, this only seemed necessary for Rails 3, so I moved it to the rails 3 specific code.

Broken Tests
I noticed that this commit in 2014 seemed to have broken some of the rails asset pipeline specs. I kept wondering why my debug code wasn't executing while trying to write a new spec and as it happens, there was no yield in run_jasmine_server block 馃槵. So, any test that only had assertions within the run_jasmine_server block were passing as potential false positives.

The test changes I have committed at this point are necessary to get the tests to pass after adding the yield. With newer versions of rails, default is to always append asset digests now, which explains why I modified the regex to support the .self-blahblahblah.js format.

Next Steps

I did verify all tests pass in rails 3/4/5, in addition to the manual tests I ran to get the screenshots above.

I'm happy to add in some tests for sprockets 4 if there's a chance this can be merged. Are there any specific recommendations on how to do that, since the Gemfile would need to be customized for those specific tests? If not, I can figure out a solution myself.

Thank you for your time and for reading this lengthy PR 馃檱!

@mgodwin
Copy link
Contributor Author

mgodwin commented Feb 3, 2018

Oh yeah! This may also fix #295, since we're looking specifically at the asset extension in the expander code now. Unfortunately I was unable to reproduce the behavior in the ticket - but I would think that if there is a reproducible issue there, this would fix it.

@slackersoft
Copy link
Member

This looks pretty good. I would like to see some tests for Sprockets 4 support, possibly just a new bit in the travis CI matrix and Gemfile that will install it. Just to prove it will continue to work. Probably best to point at the beta release on Rubygems instead of github though.

Additionally, since Jasmine 3.0 removes support for Rails 3 (which isn't supported by the rails team anymore either), you should be able to clean all that out of the asset expander as well.

Please update this PR against master/3.0 and we'll take another look. Thanks for using Jasmine!

This changes the gem to more closely follow what sprockets-rails does to resolve
asset paths.
@mgodwin
Copy link
Contributor Author

mgodwin commented Feb 23, 2018

@slackersoft Alright, rebased off master/3.0 and I added in a new test for sprockets 4. Let me know your thoughts - happy to adjust/change if you have recommendations as to how I could do it better! Thanks.

@slackersoft slackersoft merged commit 7472ffa into jasmine:master Feb 27, 2018
slackersoft pushed a commit that referenced this pull request Feb 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants