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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 theassets/
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 thedebug
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 thejavascript_include_tag
andstylesheet_include_tag
helpers. This seemed like a good decision for a couple of reasons: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:
And after this PR is introduced - you'll can see the source files are notated as
.source.js
: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: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 馃檶 :(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 ofjasmine-gem
and addgem 'sprockets', github: 'rails/sprockets'
to get Sprockets 4. After initializing jasmine in the repo, you should be able to runrake 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 paramYou 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 thedebug: true
option. So all assets had the query string param appended twice: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
inrun_jasmine_server
block 馃槵. So, any test that only had assertions within therun_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 馃檱!