css sourceMappingURL is broken

akvadrako commented 9 years ago

I'm trying to use this plugin with CSS source maps, but I can't get it to work in any fashion because this plugin doesn't understand the double extension. With this config:

tree = assetRev(tree, {
  extensions: ['js', 'css', 'png', 'jpg', 'gif', 'map'],
  exclude: ['fonts/169929'],
  replaceExtensions: ['html', 'js', 'css'],
});

I get this css file:

/*# sourceMappingURL=main-aafff1edf19e012de9347224d8cd4856.css-e997028ad01268b4cda33c1c4ce89b36.map */

but the source-map is renamed to main.css-e997028ad01268b4cda33c1c4ce89b36.map

Other combinations have similar results - even ignoring css.map doesn't help.

rickharrison commented 9 years ago

To be honest, I don't have much experience with sourcemaps. @ef4 Do you have any thoughts on this?

rickharrison commented 8 years ago

Would you be willing to contribute a failing test case for this? Once I have a failing case, I can work on fixing it.

sathappan commented 8 years ago

@rickharrison @ef4 I can confirm that this is still happening. CSS SourceMap files have a double extension of .css.map. The issue happens when getting the file extension at https://github.com/rickharrison/broccoli-asset-rev/blob/master/lib/fingerprint.js#L103. Path module's extname gets only the last extension ".map" and not ".css.map". This makes the newPath to be "main.css-e997028ad01268b4cda33c1c4ce89b36.map". But in effect it should be "main-e997028ad01268b4cda33c1c4ce89b36.css.map". sourceMappingURL expects the SourceMap file to be in the later format.

glhewett commented 7 years ago

Hi @rickharrison,

Thank you for maintaining the project. In my limited use, it seems to work fairly easily out of the box. I ran into this sourcemap issue, so I wanted to create the test as you recommended. I could probable fix it, too, if you want to provide some feedback.

I think that @sathappan has a good point, and that is one option for fixing the issue, but replacing node's path module may not be very easy. Another option would be to try and prevent links from being replaced twice. From the test, you will see that the map url gets replace, but then it triggers a match on the css replace, which creates an obviously incorrect path.

You can find my tests here: https://github.com/rickharrison/broccoli-asset-rev/compare/master...glhewett:65-css-sourcemaps

Thanks, and let me know how I can help.

glhewett commented 7 years ago

I also wanted to mention that a JS sourcemap test would be beneficial as well. I would assume that it might be suffering the same fate as the CSS sourcemap.

rickharrison commented 7 years ago

Thanks for creating a failing test case. Can you open up a PR with the test case so others can see it? For full transparency, I am not going to have time to work on this in the near future. Busy with work, holidays, etc.

glhewett commented 7 years ago

@rickharrison, thanks for setting expectations. Any feedback you have would be helpful, I am sure I can fix it eventually. Below is the output of the failing tests.

I have create a pr: #113

  1) broccoli-asset-rev revs the assets and rewrites the source:

      AssertionError: styles-6a9c5719fd509d895b7a50a972ed363e.css: does not match expected output
      + expected - actual

         width: 50px;
         height: 50px;
         background-image: url('images/sample-1f6b78f1b4667adc7e397f7bf94041ab.png');
       }
      -/*# sourceMappingURL=styles-6a9c5719fd509d895b7a50a972ed363e.css-21087e74e657901034e5668e5b1a5d59.map */
      +/*# sourceMappingURL=styles.css-21087e74e657901034e5668e5b1a5d59.map */

      at tests/filter-tests.js:24:12
      at Array.forEach (native)
      at confirmOutput (tests/filter-tests.js:18:17)
      at tests/filter-tests.js:51:7
      at tryCatch (node_modules/rsvp/dist/rsvp.js:538:12)
      at invokeCallback (node_modules/rsvp/dist/rsvp.js:553:13)
      at publish (node_modules/rsvp/dist/rsvp.js:521:7)
      at flush (node_modules/rsvp/dist/rsvp.js:2373:5)
      at _combinedTickCallback (internal/process/next_tick.js:67:7)
      at process._tickCallback (internal/process/next_tick.js:98:9)

  2) broccoli-asset-rev revs the assets when it is not the first plugin:

      AssertionError: styles-6a9c5719fd509d895b7a50a972ed363e.css: does not match expected output
      + expected - actual

         width: 50px;
         height: 50px;
         background-image: url('images/sample-1f6b78f1b4667adc7e397f7bf94041ab.png');
       }
      -/*# sourceMappingURL=styles-6a9c5719fd509d895b7a50a972ed363e.css-21087e74e657901034e5668e5b1a5d59.map */
      +/*# sourceMappingURL=styles.css-21087e74e657901034e5668e5b1a5d59.map */

      at tests/filter-tests.js:24:12
      at Array.forEach (native)
      at confirmOutput (tests/filter-tests.js:18:17)
      at tests/filter-tests.js:67:7
      at tryCatch (node_modules/rsvp/dist/rsvp.js:538:12)
      at invokeCallback (node_modules/rsvp/dist/rsvp.js:553:13)
      at publish (node_modules/rsvp/dist/rsvp.js:521:7)
      at flush (node_modules/rsvp/dist/rsvp.js:2373:5)
      at _combinedTickCallback (internal/process/next_tick.js:67:7)
      at process._tickCallback (internal/process/next_tick.js:98:9)

simonc commented 5 years ago

Hi there. Bumping this up as I encountered the issue today. Any fix or workaround since 2016? Thanks ❤️

ef4 commented 5 years ago

To give people some context and expectations: broccoli-asset-rev mostly works OK and so people keep using it. But it has some pretty fundamental problems and AFAIK nobody is actively working on rewriting it to something better.

(Well, that's not exactly true. I'm working on Embroider which eliminates the entire need for broccoli-asset-rev, but that is probably not the answer you're looking for.)

The fundamental problems are:

it doesn't take dependency graph order into account when rewriting assets. Renaming file1 can cause the hash of file2 to change if file2 refers to file1. So to be correct, you'd need to handle file1 before file2. But that doesn't happen, it just processes files in arbitrary order.
the way references to other files get rewritten via string replace often works, but is not reliably correct. There are plenty of ways it can and does break. You can accidentally replace a string you didn't mean to, and you can easily miss replacing strings that are built up dynamically.

These are both solvable, but they probably involve a rewrite that thinks about the problem differently than broccoli-asset-rev does.

This particular bug is caused by the second problem. If you first replace main.css.map with main.css-e997028ad01268b4cda33c1c4ce89b36.map and then replace main.css with main-aafff1edf19e012de9347224d8cd4856.css, you get a nonsense answer.

If I was to suggest a workaround for this particular bug, it would be to see if you can get your sourcemap named something completely different, so that the name of the source file doesn't appear inside the name of the source map file.

simonc commented 5 years ago

@ef4 Thanks for your answer!

Regarding point 1 I suppose some sort of topological sorting could help but like you said it would require to rethink the whole behavior.

Regarding point 2 I will try to see if I can change the map names I'm using rollup and broccoli-sass-source-maps so I'll have to dig a bit to see if that's even possible. I'll post an update if I do find a solution.

ember-cli / broccoli-asset-rev

css sourceMappingURL is broken #65