Fix source maps for @imported sass files w/ sassc processor #391
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @arthurnn (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Ah, the lack of |
|
I've run into a bug with sassc still reporting bad source urls. This PR's on hold for a little bit until I figure out what's going wrong. |
TylerHorth
force-pushed
the
fix/sassc-import-source-maps
branch
from
30986f2 to
067efa4
Compare
|
Alright the problem was that the SourceMapProcessor resolves each source. This means that each source must be absolute, or be in the load paths. Absolute paths are fine according to the source map spec, but otherwise they must be relative to the map file. This means that SourceMapProcessor accepts some malformed maps (the issue in #390, where sources are logical paths) and fails when given some properly formed maps (when sources are relative to the map file, but not included in the list of assets paths). The proper solution to this would probably be to modify the SourceMapProcessor to work with relative paths, and fix the processors currently outputting malformed maps (instead of the global solution in #390). To me this seems out of scope for this PR, so I have modified the sassc processor to output sources with absolute urls. I'm not sure why the one sassc test is failing, the source maps seemed to be generating properly. I'll do some testing tomorrow. |
| @@ -24,7 +24,7 @@ def call(input) | |||
| engine = Autoload::SassC::Engine.new(input[:data], options) | |||
|
|
|||
| css = Utils.module_include(Autoload::SassC::Script::Functions, @functions) do | |||
| engine.render | |||
| engine.render.gsub(/^\/\*# sourceMappingURL=.*\*\/$/, '').strip | |||
There was a problem hiding this comment.
The solves the omit_source_map_url problem until the option makes into the next sassc version.
|
can you squash your commits please? |
TylerHorth
force-pushed
the
fix/sassc-import-source-maps
branch
from
6e8940a to
d30efa6
Compare
|
Rebased and fixed an incompatibility with the |
|
Thank you very much for this!!! |
|
Thanks for this, you're on a roll. Let me know if you have any questions about anything. Appreciate your help. |
Currently, source maps are generated by the sassc engine and inlined, then the sassc processor strips them out and decodes them. As a consequence to this process the source urls are not relative to the map file, and as such they are discarded. Thus we don't get content maps for any but the root level file.
The simplest solution to this problem is to forgo the inline shenanigans and use source maps natively from libsass. I updated the sassc gem with a source map getter which will allow us to do this.
engine.source_mapmade it into sassc version1.10.1, unfortunately theomit_source_map_urloption didn't. Without this a uselesssourceMappingURLcomment is added to the bottom of the debug file just before the actual url. However, I don't believe this will be a problem since the url won't resolve to anything and thus should be ignored.The test will likely fail until #390 is merged.