search

ember-cli / broccoli-asset-rev

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

Allow custom manifest files path #44

Closed ShogunPanda closed 9 years ago

ShogunPanda commented 9 years ago

Hi!

I've added the possibility to specify a custom path (inside the destination directory) for the assets map and the Rails manifest JSON files. I also switched to pretty JSON for these files to make them human readable.

I also fixed a bug for Rails manifest file that doesn't check for the existence of the destination directory before attempting the write.

Hope this helps! Paolo

bryanaka commented 9 years ago

@ShogunPanda can you look at #47 and tell me if this is what you had in mind for this PR?

ShogunPanda commented 9 years ago

No, that's different. In your PR you're detecting existing manifest and making sure broccoli includes them in the new output. In this PR, instead, I first make sure destination directory exists (which was a bug) then I allow to customize the manifest name in case (like mine) you need something without a hash or not named manifest.json.

I think the two PR can coexist :)

bryanaka commented 9 years ago

ah, makes sense. :+1: to this PR. The only thing I'm unsure of is pretty printing, since in all cases I know of, this is meant to be consumed and altered programmatically and not by a human, as the case with bower or package files.

It would probably help @rickharrison if you added tests around this. I will probably open a PR to add the proper .travis.yml file as well unless @rickharrison beats me to it.

rickharrison commented 9 years ago

I need to look at all the contributions around this. I would like to reconcile all the different types of asset maps. I would like to decouple this plugin from any specific frameworks like rails/sprockets. I'd like to consolidate down to one format and then we can have other broccoli plugins to transform the manifest as needed.

As far as travis goes, I'm not sure what's wrong there as I use circle in my day job.

bryanaka commented 9 years ago

Hm, sounds like a good fit for some kind of adapter pattern perhaps. Though, most I've seen adhere to either the sprockets style manifest or the asset map manifest you currently have implemented.

Don't worry about the travis stuff, I'll take care of it and open a PR soon.

ShogunPanda commented 9 years ago

Should we close this one then?

rickharrison commented 9 years ago

I changed my mind. I am fine with this change. Could you rebase and add a test for the new options you are proposing?

ShogunPanda commented 9 years ago

Sure, will do ASAP.

rickharrison commented 9 years ago

I'm going to close this for now. Please re-open if you are able to finish!

ShogunPanda commented 9 years ago

Ok, I'm gonna go on vacation in two weeks and I should be able to provide what is missing for this. Sorry for the ridicolously long waiting time. :)

ShogunPanda commented 9 years ago

I created a new PR here with updated and tested code here: #78