sebdeckers / grunt-rev

:punch: Asset revving for Grunt.js
MIT License
240 stars 54 forks source link

Added alt-function to support RetinaJS libraries. #21

Closed mblarsen closed 2 years ago

mblarsen commented 10 years ago

This pull requests implements an alt(ernative) option. This options will allow to map the hash prefix from an original file - say logo.png => ab47.logo.png - onto alternative versions of that file - say logo@2x.png => ab47.logo@2x.png.

This is useful when you are working with retina images, that require the files to named identically except for some part of the filename, for example @2x.

Request included passing unit tests.

steveluscher commented 10 years ago

Continuing discussion from #15

Neat!

One bug and one API suggestion.

Bug: We must calculate the hash of the group of files. If I change one image or the other, the hash should change.

API suggestion: In my opinion, calculating a groupwise hash on files with the “@\dx” or “_\dx” suffix should be the default. Files will not likely get named as such by coincidence. Hence, I think the only option we need to add is a pattern, that when matched and eliminated, maps to some “root” filename.

Given this:

rev: {
  options: {
    // Default. RegExp or array of RegExps.
    // Matches @2x, _3x, @4x, _5x, etc…
    alternatesPattern: /[@_]\dx/
  },
  files: {
    src: ['**/*.{js,css,png,jpg}']
  }
}

The following filenames:

 image.jpg
 image@2x.jpg
 image@4x.jpg

…will map to this “root” filename:

image.jpg

All files in the group should then get hashed together to produce:

 {hash}.image.jpg
 {hash}.image@2x.jpg
 {hash}.image@4x.jpg
mblarsen commented 10 years ago

Good points. I'll add these changes.

mblarsen commented 10 years ago

I'll add an option to turn of handling of alternates.

mblarsen commented 10 years ago

I purposely used globs in stead of regexp, because I thought it would be easier for most. But the solution would be easier just using regexp.

I'm kind of for simplifying the solution.

Cons:

Pros:

mblarsen commented 10 years ago

Check the new implementation. Works as to your specs.

steveluscher commented 10 years ago

Cool.

If it were me, I'd still have /[@_]\d+x/ be the default alternatesPattern, and I would not offer a flag for turning alternates on or off – simply set alternatesPattern to null to disable grouping.

alternatesPattern: /[@_]\d+x/            // Default
alternatesPattern: [/-hover/, /-active/] // An array of patterns
alternatesPattern: null                  // Turn alternate matching off    

Again, just so everyone's clear, the role of the pattern is to produce a match which, when removed from the filename, produces some ‘base’ filename that a group of alternates share in common. Those member files then get hashed as a group and the same hash gets applied to all of the files in the group.

So…

image.jpg
image-hover.jpg
image-active.jpg

…would all produce the base filename image.jpg after the matches produced by [/-hover/, /-active/] are removed.

mblarsen commented 10 years ago

I've removed alternates: true|false and replaced it with alternatesPattern: null

The reset worked already. Added test to show default case. There are not tests for array.

Note that [/-hover/, /-active/] can be achieved by /-(hover|active)/ as well, but some times it can make it more readable to split it up.

steveluscher commented 10 years ago

Neat. Now we just need to get @cbas' opinion.

mblarsen commented 10 years ago

Come to think of it. In the current implementation the array option is seen as patterns describing separate groups alternates - different schemes you could say:

alternatesPattern: [ /-hover/, /-active/ ]

Where as:

 alternatesPattern: [ /-(hover|active)/ ] // .. or
 alternatesPattern: /-(hover|active)/

Will would describe only one pattern of alternates.

A more practical use of array would be:

alternatesPattern: [ /-(hover|active)/, /[@_]\dx/ ]

The current implementation fails if two patterns matches the same file. An example

logo.png               // matched by both
logo-hover.png
logo-action.png
logo_2x.png
logo_4x.png

In real life I guess your pattern would be:

alternatesPattern: /-(hover|active)[@_]d\x/
mblarsen commented 10 years ago

Just to let you know. We are using this problem free in production now.@cbas @steveluscher

steveluscher commented 10 years ago

:+1:

thiagofesta commented 10 years ago

:+1: I'd love to see it merged in and released as a new version... What we are waiting for? @cbas @steveluscher @mblarsen

steveluscher commented 10 years ago

I'm down, but I don't have write access to this repo. If this PR looks good to @cbas, it looks good to me!

thiagofesta commented 9 years ago

@cbas is not merging it, so for who wants to use it, replace grunt-rev with this one:

"grunt-rev": "https://github.com/TinyCarrier/grunt-rev/tarball/c8714c9a21ffd107c72a2a2db64cda9cfcd59f37"