Add output file timestamps for Gulp 4.

gulp-community / gulp-less

A LESS plugin for Gulp

559 stars 116 forks source link

Add output file timestamps for Gulp 4. #303

Closed bingnz closed 4 years ago

bingnz commented 6 years ago

Fixes #301. This modification sets the atime and mtime timestamp properties so that Vinyl will set the file times on disk correctly. This happened by default in earlier versions of Gulp, but is no longer automatic in Gulp 4.

yocontra commented 6 years ago

Going to hold off on this as https://github.com/gulpjs/vinyl/issues/105 needs to be done to fix the root issue of atime/mtime behavior.

If you're interested in fixing this in gulp 4 you can lend a hand on that ticket.

bingnz commented 6 years ago

Thanks @contra. I would like to help out if I can find some time; I've been taking a look at some of the things the Gulp team are doing around the better-stats repo and so on. I don't think I have enough context to know what actually needs to be done to get it across the line but if you have any pointers or even a high-level list of tasks I'm happy to chip away at some and I'm sure other people would too. Things like adding some tests for better-stats perhaps? Trying to integrate it with vinyl?

yocontra commented 6 years ago

@bingnz Here's a thread about it: https://github.com/gulpjs/vinyl-fs/pull/143

Basically the way I see it working is:

better-stats would be created to do exactly that - a better stats object, and it would handle the ctime (mode change) attribute automatically any time it was changed
a vinyl instance would handle updating its stats atime (access time) and mtime (modify time) whenever the contents attribute was modified (Buffer.is to compare), or stream was piped to something (would be tricky to determine if the output as actually changed or not, but possible). Would probably want to use a proxy here.
vinyl-fs would need to be modified to work with the new stat objects, and support setting ctime/mtime/atime properly on write in dest.

WraithKenny commented 4 years ago

It's been a while, and although I think many of us would like to help with the upstream, but find it all beyond us, I think it's time to fix this here instead.

https://github.com/gulpjs/gulp/issues/2193#issuecomment-618096132 Points out that this is indeed the correct "Gulp way" to do this.

https://github.com/dlmanning/gulp-sass/pull/763#issuecomment-618322577 gulp-sass has decided to merged this same solution.

(A slight difference is also setting ctime in addition ala file.stat.atime = file.stat.mtime = file.stat.ctime = new Date(); but otherwise, I think this patch is good to go.)

yocontra commented 4 years ago

Sorry but this really needs to be done outside of the plugin (https://github.com/gulpjs/vinyl/issues/105). I'm personally going to take ownership of fixing that ticket in core, this issue has persisted for a lot longer than anticipated.

@WraithKenny I think you misinterpreted his comment, that isn't the gulp way - I'm the original author of gulp and can confirm definitively that it needs to be fixed in core. Every plugin author should not need to re-implement this same behavior - making life easier for plugin authors has always been a huge priority for me.