search

ember-cli / broccoli-asset-rev

Broccoli plugin to add fingerprint checksums and CDN URLs to your assets
MIT License
87 stars 84 forks source link

Make broccoli-asset-rev run before ember-asset-loader #114

Closed Cryrivers closed 7 years ago

Cryrivers commented 7 years ago

ember-asset-loader generates asset lists by walking through the post-process tree, which needs broccoli-asset-rev to run before ember-asset-loader to make sure all files have been fingerprinted in production build before the asset list compilation.

codecov-io commented 7 years ago

Current coverage is 96.07% (diff: 100%)

Merging #114 into master will not change coverage

@@             master       #114   diff @@
==========================================
  Files             3          3          
  Lines           153        153          
  Methods          15         15          
  Messages          0          0          
  Branches         36         36          
==========================================
  Hits            147        147          
  Misses            6          6          
  Partials          0          0

Powered by Codecov. Last update 5b3c480...8cce457

rickharrison commented 7 years ago

@nathanhammond Would you mind weighing in here? I am not fully up to speed on all the ember addon stuff.

Cryrivers commented 7 years ago

@rickharrison I already pinged @nathanhammond and @trentmwillis on slack

trentmwillis commented 7 years ago

I think this will only work for simple use cases. I think broccoli-asset-rev should actually run after ember-asset-loader, but it needs to run replacements on the asset-manifest.json file that gets generated. If we have this run before, then ember-asset-loader won't include any potential prepend information.

The reason this works for simpler cases is because broccoli-asset-rev renames the output files, thus when ember-asset-loader generates a manifest by walking the directories, the files have been properly renamed. When using prepend, however, that information isn't part of the file name.

Cryrivers commented 7 years ago

@trentmwillis yes, I totally agree with you. @rickharrison can we add json to replaceExtensions?

nathanhammond commented 7 years ago

The .json can be specified by the user of the addon, doesn't need to be a default.

nathanhammond commented 7 years ago

See also: https://github.com/rickharrison/broccoli-asset-rev/issues/111

Cryrivers commented 7 years ago

@nathanhammond @trentmwillis I think I'm closing this PR in favor of manifest prefixing.

Thank you guys.