sindresorhus / gulp-rev

Static asset revisioning by appending content hash to filenames: `unicorn.css` → `unicorn-d41d8cd98f.css`
MIT License
1.54k stars 217 forks source link

Split `rev` and `rev.manifest` #184

Closed lukeed closed 7 years ago

lukeed commented 8 years ago

What's new?

Scattered across multiple issues & PR comments, I've seen multiple suggestions that rev() and rev.manifest() should be separated into two plugins. This PR took action on those suggestions.

I have the rev.manifest() code over at lukeed/gulp-rev-manifest, as well as the integration guide. I've updated the readme/documentation in both places.

Why?

Allows and clarifies that rev's sole purpose is to generate a revHash & attach it and revOrigPath to the file object. All rev-* plugins require this premise, without necessarily requiring the manifest step.

Similarly, separates all manifest-related code to its own plugin for those who want it.

How to Use?

Anywhere rev.manifest() was used, revManifest() (or assigned variable) can be used instead.

All API & functionality has been preserved.

gulp.task('example', function () {
  return gulp.src('src/*.css')
    .pipe(rev())
    .pipe(revManifest('custom-name.json', {merge: true}))
    .pipe(gulp.dest('dist'));
});

Note: For now, the manifest module is named gulp-revmanifest. I've already contacted the owner of gulp-rev-manifest (abandoned) and kindly requested an unpublish. I'd be okay with waiting on the owner's response before merging.

Reviewers

@sindresorhus, @bobthecow, anyone else


If merged, should be released as 8.0.

mention-bot commented 8 years ago

By analyzing the blame information on this pull request, we identified @sindresorhus, @bobthecow and @c0 to be potential reviewers

bobthecow commented 8 years ago

Since this is really a "first party" third party integration, if we go this route (and I think we probably should) I'd prefer that the source repo live in @sindresorhus' account too.

lukeed commented 8 years ago

Happy to transfer. I left him as author & license owner since I expected it to be the case -- not to mention that of course it's still his code.

sindresorhus commented 8 years ago

I don't really see the benefit of splitting this out. The .manifest() is here since that's the main use-case for most. Doing this would just make revving even harder than it currently is.

bobthecow commented 8 years ago

I think the main benefit is a simpler answer to the "what is gulp-rev?" question. It may or may not be worth the added complication :)

lukeed commented 8 years ago

IMO, gulp-rev: Generate a content-hash, append (or wherever) it to a new file, and save the relationship between the original file, the hash, and the hashed file.

All other rev-plugins: Access that relationship information & do something with it.

mbrevda commented 8 years ago

+1. For a myriad of reasons (namely: race conditions when writing multiple files to the manifest simultaneously) I have swapped out rev.manifest() for my own custom handler. It would make sense to keep rev()ing separate from persistence as they are essentially two different things and the functionality is not necessarily coupled between the two functions. Thanks!

lukeed commented 8 years ago

Revisiting. I still believe this is the way to go.