Add source map on debug mode with Sprockets 4 #162
Conversation
lib/sassc/rails/template.rb
Outdated
| map = input[:metadata][:map] | ||
| end | ||
|
|
||
| context.metadata.merge(data: css, map: map) |
There was a problem hiding this comment.
map metadata wasn't transmitted to sprockets and wasn't aware of dependencies, so it wasn't able to generate the source map with complete data.
|
This branch does make SCSS sourcemaps work for me with sprockets 4 (in Rails). Without the fix in this branch, dev tools debugging of SCSS was pretty impossible in sprockets 4; I had to disable source maps in my browser. (The source maps resulted in unusable mapping, where Chrome just though every line of CSS came from an Thanks @GCorbel, amazing work. I would love to see this merged and in a sassc-rails release, so I can re-enable use of source maps in Chrome. (Still very mystified why this hasn't been a higher-profile issue; is nobody else using SCSS and sprockets 4? That seems hard to believe, as they are both standard installs in a new rails app. Mystified!) |
lib/sassc/rails/template.rb
Outdated
| @@ -34,13 +34,36 @@ def call(input) | |||
| } | |||
| }.merge!(config_options) { |key, left, right| safe_merge(key, left, right) } | |||
|
|
|||
| if Rails.application.config.assets.debug && Sprockets::VERSION.start_with?("4") | |||
There was a problem hiding this comment.
Perhaps to be forward compatible this should be written in a way that it will still be enabled for sprockets 5 and other >4? It seems likely this will continue to be needed -- if it causes problems, sassc-rails will be broken with sprockets 5 either way, so doing this even in Sprockets 5+ seems the most likely to lead to things keeping working when/if a sprockets 5 is released.
There was a problem hiding this comment.
I checked and there are some options :
Sprockets::VERSION.to_i >= 4
Gem.loaded_specs['sprockets'].version.segments.first >= 4
!Sprockets::VERSION.start_with?("3")
Sassc-rails doesn't support sprockets < 3 so, because the last line is the simplest, I think this is the better option.
test/sassc_rails_test.rb
Outdated
| @@ -243,6 +245,18 @@ def test_allows_for_inclusion_of_inline_source_maps | |||
| assert_match /sourceMappingURL/, asset | |||
| end | |||
|
|
|||
| def test_adds_source_map_in_debug_mode | |||
| if Sprockets::VERSION.start_with?("4") | |||
There was a problem hiding this comment.
I think you should definitely write the test so if sprockets 5 is ever released, the test will be run under sprockets 5 too and raise on failure, instead of just skipping this test!
There was a problem hiding this comment.
I changed to make this working with Sprockets >= 4
|
@jrochkind thanks for your review. I applied your comments and did some refactoring. |
|
Would love to see this fix deployed in a new release. Debugging sass in a complex app is pretty painful as things stand. |
|
I ran into this issue today, and fixed by using the code in this PR:
I would +1 merging and cutting a new release, as a result. (I am not a maintainer of this project, just a user.) |
This fix #161. With Sprockets 4, when debug mode was enabled, the source map wasn't added correctly at the end of the file.