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

A few suggestions to simplify and fix some issues #166

Closed zhouzi closed 7 years ago

zhouzi commented 8 years ago

Hey there!

At the company I work for we recently started to create revisions and first went with gulp-rev but we quickly came across a few issues, the biggest one being parallel builds. I'd be glad to contribute to gulp-rev to fix those and I have a few other suggestions that could simplify its use but involve some breaking changes.

So in the end there's only one option left for manifest() which would be the path to the .rev files to delete old revisions. I've implemented all this in gulp-revise so have a look if you're curious but I'd be glad to see it in gulp-rev and drop gulp-revise.

Let me know what you think so I eventually start working on a fork.

sindresorhus commented 8 years ago

@tjsail33 @rajzshkr @benbieler @bobthecow @ajaybc @lukeed @samarpanda Thoughts?

lukeed commented 8 years ago

gulp-rev should create a .rev file for each revised one, which would fix the issue regarding parallel builds as they're not writing to the same file.

I think the same should be accomplished by nesting directing into the File object; eg file.revCurr, or similar. Removes the need to ignore or remove the files upon completion.

So in the end there's only one option left for manifest() which would be the path to the .rev files to delete old revisions.

The rev-manifest.json file should still be generated. I believe the majority of people are integrating that file into their applications' handlers.. else they'd be using something like rev-replace.

That said, the options object on rev.manifest() provides a lot of functionality and room for the 1% of outliers who aren't using the plugin at face value. A decent chunk of the past issues are/can be solved with these options.


So... the parallel part is nice. And that can just be accomplished by pointing to a file.revCurr.

zhouzi commented 8 years ago

I don't really understand the part regarding file.revCurr, do you mean that you could add it as a property of the file object? The thing is that the files are created from different tasks that do not end at the same time, leading to an overwrite of the rev-manifest.json's content (or at least an incomplete list of revisions). It means that each tasks has to store its "rev" somewhere so it can be accessible when the rev-manifest.json can be created (not necessarily in a file in fact).

lukeed commented 8 years ago

Oh, you're right. My head was in a different place. That revCurr property wouldn't persist beyond the original stream / handler.

Those .rev files would need to remain, as you've suggested.

I'm of the opinion that the current rev.manifest() task should be its own plugin (as is @bobthecow and others), whose basic function is that of merge() from your gulp-revise. However, I still strongly believe that the {options} object is a must-have.... for path &/or formatting transformations if nothing else.

weisk commented 8 years ago

+1 to this, i'm getting unreliable manifests while running something like:

gulp.parallel(hashJs, hashCss, hashSvg)

For now I've resorted to using gulp.series, but obviously for a performance drop. I'd love to see this implemented in gulp-rev before switching libraries.

zhouzi commented 7 years ago

I'm closing this discussion, there are open issues about the bugs that need to be fixed.