search

ember-cli / broccoli-asset-rewrite

Broccoli plugin to rewrite a source tree from an asset map.
MIT License
10 stars 53 forks source link

failing test for css/js source maps #49

Open glhewett opened 7 years ago

glhewett commented 7 years ago

@rickharrison, I took a few minutes yesterday to take a peek at a fix for the css sourcemap issue when the sourcemap name contains the css name. (application.css => application.css.map)

I am thinking that the best place to fix it is in this project, and here are some broken tests. When I get some time, I will dig into the rewrite strategy for this case.

codecov-io commented 7 years ago

Current coverage is 100% (diff: 100%)

Merging #49 into master will not change coverage

@@           master   #49   diff @@
===================================
  Files           1     1          
  Lines          85    85          
  Methods        12    12          
  Messages        0     0          
  Branches       20    20          
===================================
  Hits           85    85          
  Misses          0     0          
  Partials        0     0

Powered by Codecov. Last update 0aa76dc...09641d6

glhewett commented 7 years ago

HI @rickharrison, I have fixed the css/js source map issue by preventing a replacement of a filename in itself. For example, we will not replace styles.css in styles.css or styles-fp1.css. This will prevent the map file from being replaced twice. My PR on broccoli-asset-rev should pass after this is merged. I think we should update the broccoli-asset-rewrite dependency and merge the tests in the failing tests PR I created a couple days ago. Thanks and I hope you have/had a happy Thanksgiving.

rickharrison commented 7 years ago

Thanks for writing this fix. I believe this should be sound as I don't know why you would ever need to reference the file inside of itself. I'd like to get a second opinion from the ember cli team if you don't mind.

glhewett commented 7 years ago

Sounds good. Thanks for the response.

glhewett commented 7 years ago

Hi @rickharrison, have you had any discussion on this PR with the ember cli team? I am open to applying different strategies. I just thought this was the simplest.