Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add revision to static asset #255

Open
gpedro opened this issue Jan 8, 2015 · 24 comments
Open

Add revision to static asset #255

gpedro opened this issue Jan 8, 2015 · 24 comments

Comments

@gpedro
Copy link

gpedro commented Jan 8, 2015

References

@silvenon
Copy link
Member

silvenon commented Jan 9, 2015

There is a recipe for this, but I think you're right, maybe we should implement this.

@gpedro
Copy link
Author

gpedro commented Jan 9, 2015

@silvenon
Copy link
Member

silvenon commented Jan 9, 2015

Yeah, it would be nice if it was implemented here, too. The main problem is that I haven't figured out a way to rev all the assets, not just CSS & JS. Gulp is not very good at this stuff 😞 Otherwise I would implement it right away.

@hivre
Copy link

hivre commented Mar 19, 2015

I'm working on a solution using gulp-rev-all (see: https://github.com/smysnk/gulp-rev-all)
It looks like it can rev all assets in a smart manner.

I'll make a pull-request once I have it successfully working on my local install, should be around this weekend

@silvenon
Copy link
Member

Great! 👍

@hivre
Copy link

hivre commented Mar 20, 2015

I have a first-setup working, but there's a bug that rev-all can't rewrite references in minified html files. Also, the current setup leaves revved and unrevved files in the dist (the latter should get removed)

A simple solution would be to move the html-minify line to a separate task, and use something like gulp-napkin to remove unrevved files.

But what I'd like to propose is a little bit more involved than the most simple solution:

  • split the html task in a dedicated html task and a separate minify task and minify-html task
  • the html-task at this moment would be rather empty (but you could add html-hint etc. it also makes extending it easier)
  • the minify task will do all (and only) minification, except html files.
  • write all minified files to /.tmp
  • build reads /.tmp and revs it, then minifies html files.

I prefer this as it separates .tmp and dist more, thus not needing an additional gulp-plugin (aside from the extendability that granular separate tasks provide). and the naming of the html-task is off, it only does minification atm.
But I don't want to hijack this either, and there are probably a few other solutions available.

@silvenon
Copy link
Member

Could you post an example of how would that html task look like?

@hivre
Copy link

hivre commented Mar 20, 2015

hmm, strike that task.
I personally use it for aria and html linting, but I see that's my own addition...

So:

  • split the html task in a dedicated html task and a separate minify task and minify-html task

@silvenon
Copy link
Member

If this would help revving, I'm all for it. But I think it's best to perform it on the dist folder after the build, that way we could rev fonts and images too. Or does splitting the html task make that easier/possible?

@hivre
Copy link

hivre commented Mar 20, 2015

My vision is:

start build -> copy[*]/minify() -> dest(.tmp) -> rev(.tmp) -> dest(dist) -> minifyHtml(dist)

That way the dist always has the latest and greatest, ready for deployment. The .tmp is a temporary dir where we collect everything before we rev it and copy it to dist.
But I am also happy to see if gulp-napkin works as expected and use that to clean up the non-revved copies of files in dist, and leave the current situation as is.

[*] files we can't minify should just get copied.

@hivre
Copy link

hivre commented Mar 22, 2015

pull-request #293 uses the short-solution:

It uses gulp-rev-napkin to get rid of duplicate, un-revved files. This solution makes little changes to the gulp-file.

I have never done any testing -- I know, rather shamefull -- but I'm using this on my own project, and it works.

@sondr3
Copy link

sondr3 commented Mar 23, 2015

Have you tried using the gulp-rev-replace plugin instead of gulp-rev-napkin? Just curious since the latter hasn't been updated in a while.

@hivre
Copy link

hivre commented Mar 23, 2015

wasn't aware of gulp-rev-replace.
I'll try it out asap.

@hivre
Copy link

hivre commented Mar 24, 2015

I tried out gulp-rev-replace, but can't get it to work. I just noticed a stupid mistake on my part, will try again ASAP.

Tbh. I can see rev-napkin not being updated if it works well. it's only 111 lines of code...

@hivre
Copy link

hivre commented Mar 24, 2015

I've got something with rev-replace running, but it actually removes the revved files, and leaves the un-revved files alone. Exactly the opposite we want :)

Might be my current implementation, I'm not using a manifest file. When I did that, I got an undefined error, most likely because the file doesn't exist (is in the stream) when it's used. Probably can get around this by creating a separate task, like in the rev-replace example. But that's precisely what I'm trying not to do.

@silvenon
Copy link
Member

but it actually removes the revved files, and leaves the un-revved files alone

I was using it before and it worked fine. Maybe it gets screwed up when not replacing in the same stream as the one where revving is done?

@sondr3
Copy link

sondr3 commented Mar 24, 2015

I've been using rev-replace for a while in my optimizing task and it does exactly what it should, removes the unrevved files and inserts references to the revved ones in the HTML. It should work.

@hivre
Copy link

hivre commented Mar 24, 2015

@silvenon that's actually what I'm doing :) All in the same stream...
@sondr3 rev-all already replaces the references. I use napkin to remove un-revved files only.

I should learn how to read:

 >  "Rewrite occurences of filenames which have been renamed by gulp-rev" 

We don't need that. gulp-rev-all does this for us. Since gulp-rev-all takes child-references in account for its hashing, it's objectively better than gulp-rev (and it saves a dependency, which is a nice bonus.)

Since we export all files to /dist, and then rev it, we get:

 |-index.html
 |-index.3432.html
 |-img
   |-logo.png
   |-logo.2343.png

gulp-napkin then removes the (duplicate) unrevved files.

 |-index.3432.html
 |-img
   |-logo.2343.png

Depending on the ignore list, it removes only the files that are actually revved. Thus in my defaults it ignores index.html in revving, gulp-napkin will leave index.html untouched.

However, like I said in an earlier post, if we export all files to /.tmp and then do gulp-rev-all and export to /dist we don't need gulp-napkin.

@sondr3
Copy link

sondr3 commented Mar 24, 2015

But that's not really a problem if you properly do the tasks that will fix the assets, I never have non-revved assets in my dist-folder so gulp-napkin shouldn't be necessary at all. You need to take a look at the other tasks and maybe have them move their assets to a .tmp folder and move the revved files into the dist folder, that way you have no need for gulp-napkin.

Also, I thought that gulp-rev just renames the files itself but doesn't rename the references in the HTML, in other words you'll end up with the CSS that is being called still being named main.css while the one that you want is actually main-0b819as.css. Have you tried live serving your site after you've removed the non-revved files? AFAIK it should try to serve assets that do not exist since they reference the old files.

@hivre
Copy link

hivre commented Mar 24, 2015

But that's not really a problem if you properly do the tasks that will fix the assets, I never have non-revved assets in my dist-folder so gulp-napkin shouldn't be necessary at all. You need to take a look at the other tasks and maybe have them move their assets to a .tmp folder and move the revved files into the dist folder, that way you have no need for gulp-napkin.

I will friendly refer you to my earlier message of 4 hours ago, which literally mentions this in the last sentence, or the message of 5 days ago where I mention this as well.

Also, I thought that gulp-rev just renames the files itself but doesn't rename the references in the HTML, in other words you'll end up with the CSS that is being called still being named main.css while the one that you want is actually main-0b819as.css.

I'm not using gulp-rev, but gulp-rev-all. gulp-rev-all takes child-references into account for its hashing, therefore it's better than gulp-rev (and it saves us from the dependency on gulp-rev-replace, which is a nice bonus.)

Have you tried live serving your site after you've removed the non-revved files? AFAIK it should try to serve assets that do not exist since they reference the old files.

I can recommend that you try it as well, you'll see how remarkably well it works 😄

@sondr3
Copy link

sondr3 commented Mar 24, 2015

Yes, I do realise that, I started writing my comment as I was reading and didn't spot it before I reached the end. I am also using gulp-rev-all but whenever I disable gulp-rev-replace it doesn't rewrite the references, I just tried and without using gulp-rev-replace none of the references were replaced. Weird, I never knew gulp-rev-all was supposed to do that, I'm going to take a look again. Sorry for being a bother 😃

@hivre
Copy link

hivre commented Apr 10, 2015

I'm sorry for the non-response. I'm picking this up today

@illycz
Copy link
Contributor

illycz commented May 2, 2017

Some news about that?

@silvenon
Copy link
Member

@illycz no. 😕 We should really solve this, but Gulp isn't a good tool for this task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants