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

Fix relative urls & source maps #390

Merged
merged 4 commits into from
Oct 4, 2016

Conversation

TylerHorth
Copy link

This PR fixes two issues related to source maps and relative URLs.

  1. As reported in Source maps for files in sub-directories don't work #284, the SourceMapCommentProcessor uses the logical path as the source mapping url which is relative to the load path. However, from the source map revision 3 proposal, the url should be relative to the .debug file itself. This meant that source maps didn't work when loading assets not located in the assets root directory.
  2. A similar issue to the first popped up within the .map files. Sources should be linked relative to the .map file itself, however again they were being linked relative to the load path.

I don't believe I have the best solution for this problem, specifically the hack in source_map_utils.rb, and it's likely that there are edge cases where it breaks down, but it's necessary to get source maps functioning. I'd love to hear suggestions on ways to improve this solution.

fixes #284

@rafaelfranca @bouk

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (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.

@TylerHorth
Copy link
Author

TylerHorth commented Oct 3, 2016

Looks like I need to fix the tests.


  1) Failure:
TestSourceMaps#test_"compile babel source map" [C:/projects/sprockets/test/test_source_maps.rb:137]:
--- expected
+++ actual
@@ -1 +1 @@
-{"version"=>3, "file"=>"babel/main.js", "mappings"=>";;;;;;;;;;AACA,IAAI,IAAI,GAAG,KAAK,CAAC,GAAG,CAAC,UAAA,CAAC;SAAI,CAAC,GAAG,CAAC;CAAA,CAAC,CAAC;AACjC,IAAI,IAAI,GAAG,KAAK,CAAC,GAAG,CAAC,UAAC,CAAC,EAAE,CAAC;SAAK,CAAC,GAAG,CAAC;CAAA,CAAC,CAAC;;IAEhC,WAAW;YAAX,WAAW;;AACJ,WADP,WAAW,CACH,QAAQ,EAAE,SAAS,EAAE;0BAD7B,WAAW;;AAEb,+BAFE,WAAW,6CAEP,QAAQ,EAAE,SAAS,EAAE;GAE5B;;eAJG,WAAW;;WAKT,gBAAC,MAAM,EAAE;AACb,iCANE,WAAW,wCAME;KAChB;;;WACmB,yBAAG;AACrB,aAAO,IAAI,KAAK,CAAC,OAAO,EAAE,CAAC;KAC5B;;;SAVG,WAAW;GAAS,KAAK,CAAC,IAAI;;AAapC,IAAI,SAAS,uBACV,MAAM,CAAC,QAAQ,0BAAG;MACb,GAAG,EAAM,GAAG,EAEV,IAAI;;;;AAFN,WAAG,GAAG,CAAC,EAAE,GAAG,GAAG,CAAC;;;AAEd,YAAI,GAAG,GAAG;;AACd,WAAG,GAAG,GAAG,CAAC;AACV,WAAG,IAAI,IAAI,CAAC;;eACN,GAAG;;;;;;;;;;;CAEZ,EACF,CAAA", "sources"=>["babel/main.source-1acb9cf16a3e1ce0fe0a38491472a14a6a97281ceace4b67ec16a904be5fa1b9.es6"], "names"=>[]}
+{"version"=>3, "file"=>"babel/main.js", "mappings"=>";;;;;;;;;;AACA,IAAI,IAAI,GAAG,KAAK,CAAC,GAAG,CAAC,UAAA,CAAC;SAAI,CAAC,GAAG,CAAC;CAAA,CAAC,CAAC;AACjC,IAAI,IAAI,GAAG,KAAK,CAAC,GAAG,CAAC,UAAC,CAAC,EAAE,CAAC;SAAK,CAAC,GAAG,CAAC;CAAA,CAAC,CAAC;;IAEhC,WAAW;YAAX,WAAW;;AACJ,WADP,WAAW,CACH,QAAQ,EAAE,SAAS,EAAE;0BAD7B,WAAW;;AAEb,+BAFE,WAAW,6CAEP,QAAQ,EAAE,SAAS,EAAE;GAE5B;;eAJG,WAAW;;WAKT,gBAAC,MAAM,EAAE;AACb,iCANE,WAAW,wCAME;KAChB;;;WACmB,yBAAG;AACrB,aAAO,IAAI,KAAK,CAAC,OAAO,EAAE,CAAC;KAC5B;;;SAVG,WAAW;GAAS,KAAK,CAAC,IAAI;;AAapC,IAAI,SAAS,uBACV,MAAM,CAAC,QAAQ,0BAAG;MACb,GAAG,EAAM,GAAG,EAEV,IAAI;;;;AAFN,WAAG,GAAG,CAAC,EAAE,GAAG,GAAG,CAAC;;;AAEd,YAAI,GAAG,GAAG;;AACd,WAAG,GAAG,GAAG,CAAC;AACV,WAAG,IAAI,IAAI,CAAC;;eACN,GAAG;;;;;;;;;;;CAEZ,EACF,CAAA", "sources"=>["main.source-1acb9cf16a3e1ce0fe0a38491472a14a6a97281ceace4b67ec16a904be5fa1b9.es6"], "names"=>[]}

This demonstrates the second issue. The sources should actually be relative to the file. From the proposition:

If the sources are not absolute URLs after prepending of the “sourceRoot”, the sources are resolved relative to the SourceMap (like resolving script src in a html document).

In this case the source map file is babel/main.js.map, so a source of babel/main.source.es6 will resolve to babel/babel/main.source.es6.

@schneems
Copy link
Member

schneems commented Oct 4, 2016

I don't believe I have the best solution for this problem, specifically the hack in source_map_utils.rb, and it's likely that there are edge cases where it breaks down

I took a look at your solution and don't see a better way. My approach with source maps has been to fix bugs, add tests for them, and cut a beta release. We then wait to see if other people report bugs, and fix those without test regressions. Hopefully we'll catch the majority of edge cases before a RC.

I think i'm fine with this as is. Can you modify that failing test so that it passes? Since that is testing the case you're trying to handle, we don't need to add additional test cases.

@schneems
Copy link
Member

schneems commented Oct 4, 2016

Getting an error in the 2.3 tests

TestSourceMapUtils#test_map2:
NameError: uninitialized constant Sprockets::SourceMapUtils::PathUtils
    /home/travis/build/rails/sprockets/lib/sprockets/source_map_utils.rb:140:in `block in encode_json_source_map'
    /home/travis/build/rails/sprockets/lib/sprockets/source_map_utils.rb:139:in `each'
    /home/travis/build/rails/sprockets/lib/sprockets/source_map_utils.rb:139:in `encode_json_source_map'
    test/test_source_map_utils.rb:66:in `test_map2'

@TylerHorth
Copy link
Author

Strange that it's happening on 2.3.0 but not 2.3.1. Looking into it.

@schneems
Copy link
Member

schneems commented Oct 4, 2016

Agreed, i restarted the test to make sure there wasn't a hiccup and it is still failing.

@schneems
Copy link
Member

schneems commented Oct 4, 2016

Looks like that did it. Not sure why it failed in 2.3.0 but not 2.3.1. Pretty weird. Thanks for your work 🚀

@schneems schneems merged commit fba45fe into rails:master Oct 4, 2016
@TylerHorth TylerHorth mentioned this pull request Oct 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source maps for files in sub-directories don't work
4 participants